Introduce Hns and Flat strategy and integrate with GcsFileSystem#311
Introduce Hns and Flat strategy and integrate with GcsFileSystem#311suni72 wants to merge 15 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a bucket capabilities caching layer and namespace strategy resolution to support Hierarchical Namespace (HNS) routing and optimization. Key changes include adding BucketCapabilities, defining NamespaceStrategy with flat and hierarchical implementations, and integrating a Caffeine-backed cache with TTL in AnalyticsCacheManager. Feedback focuses on improving encapsulation and API design, specifically by removing testing-only methods from the public GcsFileSystem interface, making internal methods package-private, converting BucketCapabilities to an @AutoValue class, and simplifying a lambda expression to a method reference.
suni72
force-pushed
the
dir-metadata-ops-2
branch
3 times, most recently
from
c507032 to
c58a027
Compare
suni72
force-pushed
the
dir-metadata-ops-2
branch
5 times, most recently
from
968794a to
0ffc941
Compare
…nject BucketPropertiesLoader to GcsFileSystemImpl
…th VisibleForTesting
…tion and enhance test coverage
… GcsFileSystemImpl instantiation
suni72
force-pushed
the
dir-metadata-ops-2
branch
4 times, most recently
from
5db7b68 to
70c1223
Compare
suni72
force-pushed
the
dir-metadata-ops-2
branch
from
70c1223 to
98f664d
Compare
| FlatNamespaceStrategyImpl( | ||
| GcsClient gcsClient, Supplier<ExecutorService> statusExecutorServiceSupplier) { | ||
| this.gcsClient = gcsClient; | ||
| this.statusExecutorServiceSupplier = statusExecutorServiceSupplier; |
There was a problem hiding this comment.
Might have to think of a better name for the executor service like metadataExecutorServiceSupplier or something else.
|
|
||
| private final Telemetry telemetry; | ||
| private final AnalyticsCacheManager cacheManager; | ||
| private final AnalyticsCacheManager.BucketPropertiesLoader bucketPropertiesProvider; |
There was a problem hiding this comment.
Why do we need to take this extra parameter as the input. Can you check if their is way to avoid this? why do other cache do not require this?
| * Status calls (e.g., getting file info) are lightweight. A core pool size of 2 allows basic | ||
| * concurrency without significant resource overhead. | ||
| */ | ||
| private static final int DEFAULT_STATUS_CORE_POOL_SIZE = 2; |
There was a problem hiding this comment.
If this is not configurable by user, this is the final value. we do not need to add DEFAULT prefix.
| BucketProperties properties = | ||
| cacheManager.getBucketProperties(bucketName, bucketPropertiesProvider); | ||
|
|
||
| if (properties.isHnsEnabled() && fileSystemOptions.isHnsApiEnabled()) { |
There was a problem hiding this comment.
The check should be the other way around.
We should first check for the flag and then properties.isHnsEnabled().
properties.isHnsEnabled() involves an api call. If the flag is not on, we should not do the extra api call.
There was a problem hiding this comment.
Lets move this implementation of LazyExecutor and test into another PR, where we implement getFileInfo where it is used. Also remove the parallel execution flag, we can introduce it during the implementation of getFileInfo
|
|
||
| package com.google.cloud.gcs.analyticscore.client; | ||
|
|
||
| interface NamespaceStrategy {} |
There was a problem hiding this comment.
The interface should not be empty. It should define the methods which will need different handling between the HNS and flat bucket otherwise the purpose of the interface is defeated.
The interface should be designed in a way such that we do not expose implementation logic, while also not duplicating logic between the two
interface NamespaceStrategy {
FileInfo getDirectoryInfo(GcsItemId itemId) throws IOException;
void createDirectory(StorageResourceId resourceId) throws IOException;
void renameDirectory(GcsItemInfo srcInfo, GcsItemId dstId) throws IOException;
void deleteDirectory(GcsItemInfo dirInfo, boolean recursive) throws IOException;
List<FileInfo> listDirectory(GcsItemId id, GcsListOptions options) throws IOException;
void repairImplicitDirectory(URI parentPath) throws IOException;
----
others
}
I guess you did not add the methods here so that we do not need to unnecessarily add the empty implementations in the concrete classes.
Lets discuss which would be better approach tomorrow.
But at the very least we should have a comment that the methods will be added in the follow up PRs, else the interface looks more like a placeholder.
There was a problem hiding this comment.
Changes are unrelated to the changes in this PR. Please move them into a seperate PR.
There was a problem hiding this comment.
The changes do not seems to be related to the change. Send them in a seperate PR.
They are increasing review overhead by increasing the number of files to review.
There was a problem hiding this comment.
The changes do not seems to be related to the change. Send them in a seperate PR.
They are increasing review overhead by increasing the number of files to review.
There was a problem hiding this comment.
The changes do not seems to be related to the change. Send them in a seperate PR.
They are increasing review overhead by increasing the number of files to review.
There was a problem hiding this comment.
The changes do not seems to be related to the change. Send them in a seperate PR.
They are increasing review overhead by increasing the number of files to review.
There was a problem hiding this comment.
The changes do not seems to be related to the change. Send them in a seperate PR.
They are increasing review overhead by increasing the number of files to review.
There was a problem hiding this comment.
Same comment as above. Move them in a seperate PR.
| | Property | Description | Default Value | | ||
| | :--- | :--- | :--- | | ||
| | `analytics-core.hns.api.enable` | Controls whether the Hierarchical Namespace (HNS) API is enabled for operations. | `false` | | ||
| | `analytics-core.status.parallel.enabled` | Controls whether Cloud Storage object requests for metadata and status operations execute in parallel. | `true` | |
There was a problem hiding this comment.
Lets introduce this during the implementation phase.
2 participants
Type of Change
feat: A new featurefix: A bug fixdocs: Documentation only changesstyle: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)refactor: A code change that neither fixes a bug nor adds a featureperf: A code change that improves performancetest: Adding missing tests or correcting existing testschore: Changes to the build process or auxiliary tools and libraries such as documentation generationDescription
What?
NamespaceStrategyinterface alongsideFlatNamespaceStrategyImplandHierarchicalNamespaceStrategyImplimplementations.GcsFileSystemImplto integrate NamespaceStrategy based on bucket HNS configuration and HNS API enable flag in core.GcsFileSystemOptionsto configure HNS behavior and parallel status execution.LazyExecutorService, a lightweight version of Hadoop's LazyExecutorService. This will be used to execute list calls synchronously when parallel execution is disabled.Why?
Dynamically routes folder commands based on the bucket's namespace configuration (flat vs. hierarchical).
Checklist
feat(core): ...)Generated/Assisted by Agent? [Yes/No]