Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DVC] Add RequestBasedMetaRepository to enable metadata retrieval directly from server #1467

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pthirun
Copy link
Contributor

@pthirun pthirun commented Jan 23, 2025

[DVC] Add RequestBasedMetaRepository to enable metadata retrieval directly from server

This PR introduces the use of the /store_properties/{store_name} endpoint to retrieve and cache store metadata for the DaVinci client. The endpoint was implemented in the Venice Server in this PR: #1374.

The RequestBasedMetaRepository leverages this endpoint to maintain a cache of Store and SchemaData objects for requested stores. The cache is refreshed by the NativeMetadataRepository, which this new class extends.

By default, this feature is disabled in the ClientConfig passed to the NativeMetadataRepository. The default configuration preserves the current behavior of using the ThinClientMetaStoreBasedRepository. The relevant configuration field is ClientConfig.useRequestBasedMetaRepository. For more details, see NativeMetadataRepository.getInstance.

How was this PR tested?

Unit and Integration test included in PR.

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to explain your proposed changes and call out the behavior change.

NOTE: behavior will not change by default, feature flag in ClientConfig must be turned on.

@pthirun pthirun requested a review from xunyin8 January 23, 2025 00:46
@pthirun pthirun force-pushed the feature/dvc-request-based-metadata-01-22-2025 branch 2 times, most recently from cfa6434 to 80f0745 Compare January 23, 2025 22:29
Copy link
Contributor

@xunyin8 xunyin8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, thanks for the refactoring. Just some minor comments and also looks like test coverage is not passing.

@pthirun pthirun force-pushed the feature/dvc-request-based-metadata-01-22-2025 branch 16 times, most recently from 00fdc3a to 50d3ef2 Compare February 6, 2025 21:25
@pthirun pthirun self-assigned this Feb 6, 2025
@pthirun pthirun force-pushed the feature/dvc-request-based-metadata-01-22-2025 branch from 50d3ef2 to de203f4 Compare February 6, 2025 22:21
Copy link
Contributor

@xunyin8 xunyin8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes and unit tests, I think we are a bit lacking on the integration tests. See comments below.

getAndCacheSchemaDataFromSystemStore(storeName);
nativeMetadataRepositoryStats.updateCacheTimestamp(storeName, clock.millis());
} else {
removeStore(storeName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put a comment on NativeMetadataRepository that it does not handle StoreDataChangedListener#handleStoreDeleted events? Since the current implementation will throw exceptions during metadata fetch and not really differentiating store deleted vs unable to retrieve metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrote a comment above the method stating it will throw the VeniceNoStoreException on either deleting or missing stores. Is this an accurate representation?

nativeMetadataRepository.start();
// ThinClientMetaStoreBasedRepository implementation should be used since CLIENT_USE_META_SYSTEM_STORE_REPOSITORY
// is set to true without enabling other feature flags.
Assert.assertTrue(nativeMetadataRepository instanceof RequestBasedMetaRepository);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment doesn't match the assertion and also this is a weird place to put a test for RequestBasedMetaRepository in a file called MetaSystemStoreTest. Can we add a new test file where we run a more comprehensive integration test with a DVC? i.e push some new versions or make some store config changes and make sure the DVC pick up those changes. You can see examples like the recently added testBatchOnlyMaterializedViewDVCConsumer. You probably don't need a VeniceTwoLayerMultiRegionMultiClusterWrapper, a single region will be sufficient.

@pthirun pthirun force-pushed the feature/dvc-request-based-metadata-01-22-2025 branch from de203f4 to 277b16c Compare February 6, 2025 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants