[compat] [controller] add a field largestUsedRTVersionNumber in store config
#1512
Conversation
arjun4084346
force-pushed
the
rtnamechange
branch
3 times, most recently
from
1e2c1f9 to
e74aa24
Compare
arjun4084346
changed the title
largestUsedRTVersionNumber in store config
arjun4084346
force-pushed
the
rtnamechange
branch
from
e74aa24 to
d34bb0c
Compare
arjun4084346
force-pushed
the
rtnamechange
branch
from
d34bb0c to
a910cab
Compare
arjun4084346
force-pushed
the
rtnamechange
branch
from
a910cab to
667dd27
Compare
There was a problem hiding this comment.
Thanks, @arjun4084346 . Please take a look at my comments and let me know if you have any questions.
| return this.storeProperties.largestUsedRTVersionNumber; | ||
| } | ||
|
|
||
| @SuppressWarnings("unused") // Used by Serializer/De-serializer for storing to Zoo Keeper |
There was a problem hiding this comment.
nit: there is no space in Zoo Keeper
| @@ -314,7 +314,7 @@ subprojects { | |||
| doFirst { | |||
| def versionOverrides = [ | |||
| // project(':internal:venice-common').file('src/main/resources/avro/StoreVersionState/v5', PathValidation.DIRECTORY) | |||
| project(':internal:venice-common').file('src/main/resources/avro/StoreMetaValue/v27', PathValidation.DIRECTORY), | |||
| // project(':internal:venice-common').file('src/main/resources/avro/StoreMetaValue/v27', PathValidation.DIRECTORY), | |||
| @@ -1190,6 +1190,11 @@ public VersionResponse getStoreLargestUsedVersion(String clusterName, String sto | |||
| return request(ControllerRoute.GET_STORE_LARGEST_USED_VERSION, params, VersionResponse.class); | |||
| } | |||
|
|
|||
| public VersionResponse getStoreLargestUsedRTVersion(String clusterName, String storeName) { | |||
There was a problem hiding this comment.
Why do we need separate API for this if the route (GET_STORE_LARGEST_USED_VERSION) is the same as that of getStoreLargestUsedVersion?
| + store.getName() + ", it's smaller than one found in graveyard: " + largestUsedVersionNumber; | ||
| LOGGER.error(errorMsg); | ||
| throw new VeniceException(errorMsg); | ||
| if (largestUsedRTVersionNumber > store.getLargestUsedRTVersionNumber()) { |
There was a problem hiding this comment.
Why do we need this check for RT during store migration? It doesn't make sense to me based on the above comments. Am I missing something?
| @@ -156,7 +204,7 @@ public List<String> listStoreNamesFromGraveyard(String clusterName) { | |||
| * @return Matching store from each venice. Normally contains one element. | |||
| * If the store existed in some other cluster before, there will be more than one element in the return value. | |||
| */ | |||
| private List<Store> getStoreFromAllClusters(String storeName) { | |||
There was a problem hiding this comment.
Feel free to add com.linkedin.venice.annotation.VisibleForTesting annotation if visibility is changed for unit testing only
| if (deletedStore.getLargestUsedRTVersionNumber() > largestUsedRTVersionNumber) { | ||
| largestUsedRTVersionNumber = deletedStore.getLargestUsedRTVersionNumber(); |
There was a problem hiding this comment.
can be replaces with Math.max like one usage somewhere below in this PR
| } | ||
|
|
||
| @Test | ||
| void testStoreWithValidSystemStoreAttributes() { |
There was a problem hiding this comment.
Let's add one more test with DVC or meta system store
There was a problem hiding this comment.
Do we really need new test class? Can we use one of the existing class? Or we can consolidate all E2E tests related to RT repartitioning in one test class.
|
|
||
| for (ControllerClient controllerClient: childControllerClients) { | ||
| Assert.assertEquals(controllerClient.getStore(storeName).getStore().getCurrentVersion(), 1); | ||
| } |
There was a problem hiding this comment.
Where is the validation for largest used RT version?
| LOGGER.info( | ||
| "Found store: {} in the store graveyard. Will initialize the RT version to {}.", | ||
| storeName, | ||
| largestUsedStoreVersion); |
There was a problem hiding this comment.
should log largestUsedRTStoreVersion and not largestUsedStoreVersion
Adding a field largestUsedRTVersionNumber in store config, which will be used to create new RT topic name whenever needed in future work
When we delete and recreate a store, we do not want to use the same old real time topic name, because that might not have been fully cleaned up. We want to use a new name, and to create the new name, we need to know the previous name that was ever used. This config will help in finding this out.
Resolves #XXX
How was this PR tested?
Does this PR introduce any user-facing changes?