[da-vinci] Apply Da Vinci record transformer config to associated store only #1393
Conversation
| if (recordTransformerFunction != null) { | ||
| throw new VeniceException("DaVinciRecordTransformer doesn't support blob transfer."); | ||
| } | ||
|
|
There was a problem hiding this comment.
DVRT doesn't yet store the hash in OffsetRecord, which is needed to support blob transfer with DVRT. Once that is integrated, we can remove this check.
| StoreBackend storeBackend = getStoreOrThrow(storeName); | ||
| storeBackend.subscribe(ComplementSet.newSet(partitions), Optional.of(version)); | ||
| }); | ||
| if (autoSubscribeExistingResourcesDuringBootstrap()) { |
There was a problem hiding this comment.
Kristy is working on this currently, so we should remove it to not duplicate efforts. We should probably get her PR merged first, then rebase from there.
| if (backendConfig.getString(ConfigKeys.SERVER_INGESTION_MODE, IngestionMode.BUILT_IN.toString()) | ||
| .equals(IngestionMode.ISOLATED.toString())) { | ||
| throw new VeniceClientException("Ingestion isolation is not compatible with DaVinciRecordTransformer"); |
There was a problem hiding this comment.
Why is this needed? Is checking for daVinciConfig.isIsolated() not sufficient?
| throw new VeniceClientException("Ingestion Isolation is not supported with DaVinciRecordTransformer"); | ||
| if (backendConfig.getBoolean(ConfigKeys.DAVINCI_SUBSCRIBE_RESOURCES_DURING_BOOTSTRAP_ENABLED, true)) { | ||
| throw new VeniceClientException( | ||
| "DaVinciRecordTransformer requires turn off auto-subscribe existing resources during bootstrap"); |
There was a problem hiding this comment.
Why is this the case? When SIT is constructed, it should already have the DVRT function passed into the constructor.
| // Register store specific record transformer upon client start-up. | ||
| if (daVinciConfig.isRecordTransformerEnabled()) { | ||
| daVinciBackend.get() | ||
| .registerRecordTransformerFunction(getStoreName(), daVinciConfig.getRecordTransformerFunction()); |
There was a problem hiding this comment.
I made some changes here and daVinciConfig.getRecordTransformerFunction() has been removed. We're now just using daVinciConfig.getRecordTransformerConfig().
[da-vinci] Apply Da Vinci record transformer config to associated store only
Current code will set DVRT function to ingestion service, and all the store ingestion tasks will share the same DVRT function.
This is not correct if we are running multiple stores with some being DVRT Da Vinci client and some are normal clients.
This PR changes the assumption, and will register the function when a store client is being started.
Also, to support DVRT, we should disable automatic subscription to existing on disk resources during bootstrap stage, as it does not contain DVRT function information. We should rely on user's explicit initialization call to setup function mapping correctly during restart. This PR introduces a method to disable this auto-subscription behavior: "davinci.subscribe.resources.during.bootstrap.enabled", but the default is true, as it is the existing behavior. We will explicitly turn it off when we are spinning up DVRT Da Vinci client.
How was this PR tested?
Adjust integration test and add a new normal client to spin up along with DVRT client and verify the behavior is expected.
Does this PR introduce any user-facing changes?