Conversation
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@ivandika3 , thanks for working on this! Just have a question and a very minor comment.
| @@ -71,7 +65,6 @@ public OMDirectoryCreateResponseWithFSO(@Nonnull OMResponse omResponse, | |||
| public OMDirectoryCreateResponseWithFSO(@Nonnull OMResponse omResponse, | |||
| @Nonnull Result result) { | |||
There was a problem hiding this comment.
Let's remove the result parameter. (If no further change is needed, let's just merge this PR and fix this later.)
There was a problem hiding this comment.
Updated since there is also another review to address.
| if (cache != null) { | ||
| LOG.info("Updating cache for accessId/user: {}.", accessId); | ||
| cache.put(accessId, secret); | ||
| TableCacheUpdateTracker.recordCacheUpdate(S3_SECRET_TABLE); |
There was a problem hiding this comment.
Question: why track it here?
There was a problem hiding this comment.
The S3SecretManager uses table cache, but the implementation does not use the unified TypedTable, probably because it supports both external S3 secret store (i.e. VaultSecretStore) and embedded (OM RocksDB), but Ozone table mechanisms only support RocksDB (TypedTable uses RDBTable as the underlying store).
There is also cache related cleanup logic in OzoneManagerDoubleBuffer just for S3_SECRET_TABLE which I think it kind of hacky since the whole cache evictions has already been supported by the OM TableCache. Technically, it should be possible to unify it by making the S3SecretCache to be a TableCache, but this means that we might need to make OM to generalize the table cache and store implementation API (where the store can support other remote store like VaultStore or even things like remote KV store which means that OM only stores in-memory cache similar to QiHoo OM architecture, see https://github.com/apache/ozone/files/13347053/OM.-Qihoo.pdf), which is going to be quite involved.
|
@ivandika3 thanks for the patch! overall lg |
3 participants
What changes were proposed in this pull request?
We have supported automatic cache tracking and cleanup in HDDS-13365, but keep using CleanupTableInfo for safe guards. If HDDS-13365 works well, we can remove the CleanupTableInfo entirely.
Note this also adds a
TableCacheUpdateTracker.recordCacheUpdate(S3_SECRET_TABLE)since S3_SECRET_TABLE has a separate cache mechanisms than the normal OM table cache.This would remove one footgun from OM development (hopefully without introducing another one).
Generated by: GPT 5.6.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16643
How was this patch tested?
CI (Clean CI: https://github.com/ivandika3/ozone/actions/runs/36673297490).