-
Notifications
You must be signed in to change notification settings - Fork 3
Add namespace-creation feature #322
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @jnmt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant new capability: the ability to create and manage namespaces. This feature provides a structured way to partition and isolate resources within the system, enhancing multi-tenancy and data organization. The changes span across the client, common, and ledger modules, introducing new APIs, robust server-side logic, and updated database interactions to support this functionality. It ensures that namespace creation is validated, handled correctly, and integrated seamlessly with the existing architecture. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant new feature for namespace creation, touching multiple components across the client and ledger. The implementation is generally well-structured, with good test coverage for the new functionality. The refactoring in LedgerModule to use factories and multibindings for TableMetadataProvider is a solid improvement for dependency injection and extensibility. However, I've identified a critical race condition in the namespace creation logic that should be addressed to ensure atomicity. I've also included a minor suggestion to improve exception handling. Overall, this is a good contribution.
...er/src/main/java/com/scalar/dl/ledger/database/scalardb/AbstractScalarNamespaceRegistry.java
Show resolved
Hide resolved
ledger/src/main/java/com/scalar/dl/ledger/namespace/NamespaceManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request implements namespace creation functionality for ScalarDL, allowing clients to create isolated namespaces for organizing contracts, functions, and other resources. The implementation adds gRPC endpoints, validation logic, and comprehensive test coverage across the ledger, client, and RPC layers.
- Adds
createNamespaceRPC methods to LedgerPrivileged, AuditorPrivileged, and GatewayPrivileged services - Implements namespace name validation requiring alphabetic start and alphanumeric/underscore characters
- Extends client APIs to support namespace creation in both CLIENT and INTERMEDIARY modes
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rpc/src/main/proto/scalar.proto | Adds createNamespace RPC method and NamespaceCreationRequest message definition |
| ledger/src/main/java/com/scalar/dl/ledger/namespace/NamespaceManager.java | Implements namespace validation and creation logic |
| ledger/src/main/java/com/scalar/dl/ledger/database/scalardb/AbstractScalarNamespaceRegistry.java | Handles database namespace and table creation operations |
| ledger/src/main/java/com/scalar/dl/ledger/service/BaseService.java | Adds namespace creation service method |
| ledger/src/main/java/com/scalar/dl/ledger/service/LedgerModule.java | Updates dependency injection configuration for namespace functionality |
| client/src/main/java/com/scalar/dl/client/service/ClientService.java | Adds public API for namespace creation |
| common/src/main/java/com/scalar/dl/ledger/error/CommonError.java | Defines error codes for namespace operations |
| ledger/src/test/java/com/scalar/dl/ledger/namespace/NamespaceManagerTest.java | Comprehensive test coverage for namespace validation |
| ledger/src/test/java/com/scalar/dl/ledger/database/scalardb/LedgerNamespaceRegistryTest.java | Tests database namespace creation operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
common/src/main/java/com/scalar/dl/ledger/error/CommonError.java
Outdated
Show resolved
Hide resolved
ledger/src/main/java/com/scalar/dl/ledger/namespace/NamespaceManager.java
Outdated
Show resolved
Hide resolved
josh-wong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!🙇🏻♂️
| Retry.decorateRunnable( | ||
| retry, | ||
| () -> { | ||
| createNamespaceManagementTable(); | ||
| createNamespace(namespace); | ||
| addNamespaceEntry(namespace); | ||
| }) | ||
| .run(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retry logic is introduced mainly for Cassandra environments. Cassandra client sessions (storage, storageAdmin, transactionAdmin) have a delay to sync the namespace/table metadata with each other when creating a namespace/table, and often see "namespace/table not found" errors.
| // this sleep can be removed after negative cache is disabled in ScalarDB | ||
| Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, ScalarDB caches an empty result (negative cache) when a specified table is not found (e.g., due to the metadata sync delay mentioned above). It takes 60 seconds by default to expire, and users cannot create the namespace during that time. So, we would like to avoid it.
But, after the negative cache is disabled in ScalarDB, this delay/sleep can be achieved through the retry logic. Since sleep here is unnecessary and inefficient for other underlying storage, remove it after the fixed ScalarDB version is released.
Description
This PR adds the functionality to create multiple asset namespaces on a single ScalarDL cluster, allowing users to use separate namespaces for assets.
As the first step of the namespace feature, this PR only adds the creation interface and service. Creation CLI and the new Ledger interface to use multiple namespaces will be added in other PRs. Also, the E2E tests will be added in a PR on the auditor side.
Related issues and/or PRs
N/A
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
Added namespace creation functionality.