hemantk-12 commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1480558731
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -237,8 +242,9 @@ private synchronized void cleanup() {
* TODO: [SNAPSHOT] Add new ozone debug CLI command to trigger this directly.
*/
private void cleanupInternal() {
- for (Map.Entry<String, ReferenceCounted<IOmMetadataReader, SnapshotCache>>
entry : dbMap.entrySet()) {
- dbMap.compute(entry.getKey(), (k, v) -> {
+ for (String evictionKey : pendingEvictionQueue) {
+ dbMap.compute(evictionKey, (k, v) -> {
+ pendingEvictionQueue.remove(k);
Review Comment:
Can we add test for it? Or may be use iterator.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java:
##########
@@ -82,7 +82,7 @@ static void beforeAll() throws Exception {
void setUp() {
// Reset cache for each test case
snapshotCache = new SnapshotCache(
- omSnapshotManager, cacheLoader, CACHE_SIZE_LIMIT);
+ omSnapshotManager, cacheLoader, CACHE_SIZE_LIMIT, 0);
Review Comment:
How setting the clean up interval to 0 is working for the test? I don't
think it is correct to set clean-up to 0 or tests are more correct.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -485,7 +485,8 @@ private enum State {
private OmMetadataReader omMetadataReader;
// Wrap active DB metadata reader in ReferenceCounted once to avoid
// instance creation every single time.
- private ReferenceCounted<IOmMetadataReader, SnapshotCache>
rcOmMetadataReader;
+ private ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ rcOmMetadataReader;
Review Comment:
nit: it was indented better before.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -178,18 +203,12 @@ public ReferenceCounted<IOmMetadataReader, SnapshotCache>
get(String key, boolea
// when called from SDT (and some) if the snapshot is not active anymore.
if (!skipActiveCheck && !omSnapshotManager.isSnapshotStatus(key,
SNAPSHOT_ACTIVE)) {
// Ref count was incremented. Need to decrement on exception here.
- rcOmSnapshot.decrementRefCount();
+ rcOmSnapshot.close();
Review Comment:
This should be `decrementRefCount()` and not `close()`. Some of the
background services for exmaple `SnapshotDeletingService` works on non-Active
snapshot. Snapshot is opened by `SnapshotDeletingService`, it will close the
currently used snapshot.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -2580,8 +2581,8 @@ public boolean getAllowListAllVolumes() {
return allowListAllVolumes;
}
- public ReferenceCounted<
- IOmMetadataReader, SnapshotCache> getOmMetadataReader() {
+ public ReferenceCounted<IOmMetadataReader,
Review Comment:
nit: it could be in a single line since 120 chars are allowed per line now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
