You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ivandika3 (via GitHub)" <gi...@apache.org> on 2026/09/16 02:18:39 UTC
[PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
ivandika3 opened a new pull request, #10171:
URL: https://github.com/apache/ozone/pull/10171
## What changes were proposed in this pull request?
We need to ensure that the tables in CleanupTableInfo in OM response should be exactly the same as the ones updated in the corresponding OM request's validateAndUpdateCache. Previously, this is done with using `CleanupTableInfo` annotations on the `OMClientResponse`. However, this is hard to keep track because all the cache updates are done in the `OMClientRequest`, but we need to update the `OMClientResponse` instead, which is very error-prone. We should couple the table cache update in `OMClientRequest` and the tables to cleanup. That way, we remove the `CleanupTableInfo` reflection entirely.
One way is to track table cache to cleanup for the `OmClientResponse` whenever a cache is updated (e.g. using some context object). This will ensure that any table cache updated will eventually be cleaned up. Moreover, this can save some cleanup overhead if not all tables in the `CleanupTableInfo` need to be cleaned up in certain cases (e.g. OMOpenKeysDeleteRequest might only need to cleanup only OPEN_KEY_TABLE (if all the open keys are from OBS / LEGACY buckets) or OPEN_FILE_TABLE (if all the open keys are from FSO bucket) or both. For example, for each Table#addCacheEntry, we also need to update the list of tables to cleanup. However, there might be some memory overhead when passing the OM table cache to cleanup (although the context objects should be short-lived).
With this patch, we do not need to add any explicit `CleanupTableInfo` since all cache entry is eligible to be cleaned up as soon as it's added. This would make OM request/response development less error-prone and contributors do not need to worry about cache cleanup mechanism while developing OM request and response.
Note: Currently, the implementation uses ThreadLocal since currently OM validateAndUpdateCache only runs in a single thread (i.e. Ratis StateMachineUpdater) and the code changes is minimal. An alternative (less error-prone) idea is to pass tracker inside the `ExecutionContext` which should be safer, but will require changes in a lot more places.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13365
## How was this patch tested?
UT (Clean CI: https://github.com/ivandika3/ozone/actions/runs/25242237805)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-5842354525
@szetszwo Thanks for the review, updated accordingly.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-5885433546
> it seems the kubernetes CI kept failing (https://github.com/apache/ozone/actions/runs/36218318505/job/109238015351?pr=10171) due to unable to download the artifact.
>
> ```
> Downloading single artifact
> Error: Unable to download artifact(s): Artifact not found for name: ozone-bin
> Please ensure that your artifact is not expired and the artifact was uploaded using a compatible version of toolkit/upload-artifact.
> ```
`ozone-bin` is created by `build` check, and expires after 1 day. Checks that need it (`acceptance`, `dependency`, `kubernetes`) can be selectively re-run only within that time period. Afterwards "Re-run all jobs" is the way to go, which re-runs `build`, too.
We can increase expiry to 1 week to help with that.
For this PR, I think `kubernetes` failure can be ignored, since it passed in fork run.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #10171:
URL: https://github.com/apache/ozone/pull/10171#discussion_r3836500264
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TableCacheUpdateTracker.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.db;
+
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.Set;
+
+/**
+ * Tracks table caches updated by the current thread.
+ */
+public final class TableCacheUpdateTracker {
+
+ private static final ThreadLocal<Tracker> CURRENT_TRACKER =
+ new ThreadLocal<>();
+ private static final Set<String> TRACKING = Collections.emptySet();
Review Comment:
Since Collections.emptySet() is immutable, TRACKING is always an empty set. Let's remove it and just use null (i.e. comparing "tables == null" instead of "table == TRACKING".)
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/OMClientResponse.java:
##########
@@ -86,5 +91,28 @@ public void setOmLockDetails(
OMLockDetails omLockDetails) {
this.omLockDetails = omLockDetails;
}
-}
+ public Set<String> getCleanupTables() {
+ return cleanupTables;
+ }
+
+ public void setCleanupTables(Collection<String> tables) {
+ cleanupTables = toCleanupTables(tables);
+ }
+
+ public void addCleanupTables(Collection<String> tables) {
+ if (tables == null || tables.isEmpty()) {
+ return;
+ }
+ Set<String> merged = new LinkedHashSet<>(cleanupTables);
+ merged.addAll(tables);
+ cleanupTables = toCleanupTables(merged);
Review Comment:
merged is already a new set. Just make it unmodifiable:
```java
cleanupTables = Collections.unmodifiableSet(merged);
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TableCacheUpdateTracker.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.db;
+
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.Set;
+
+/**
+ * Tracks table caches updated by the current thread.
+ */
+public final class TableCacheUpdateTracker {
+
+ private static final ThreadLocal<Tracker> CURRENT_TRACKER =
+ new ThreadLocal<>();
+ private static final Set<String> TRACKING = Collections.emptySet();
+
+ private TableCacheUpdateTracker() {
+ }
+
+ public static Tracker track() {
+ Tracker tracker = new Tracker(CURRENT_TRACKER.get());
+ CURRENT_TRACKER.set(tracker);
+ return tracker;
+ }
+
+ public static void recordCacheUpdate(String tableName) {
+ Tracker tracker = CURRENT_TRACKER.get();
+ if (tracker != null) {
+ tracker.recordCacheUpdate(tableName);
+ }
+ }
+
+ /**
+ * Tracks updated tables until the scope is closed.
+ */
+ public static final class Tracker implements AutoCloseable {
Review Comment:
Inner class is not needed -- just move everything to TableCacheUpdateTracker.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TableCacheUpdateTracker.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.db;
Review Comment:
Let's put it to org.apache.hadoop.hdds.utils.db.cache?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/OMClientResponse.java:
##########
@@ -86,5 +91,28 @@ public void setOmLockDetails(
OMLockDetails omLockDetails) {
this.omLockDetails = omLockDetails;
}
-}
+ public Set<String> getCleanupTables() {
+ return cleanupTables;
+ }
+
+ public void setCleanupTables(Collection<String> tables) {
+ cleanupTables = toCleanupTables(tables);
+ }
Review Comment:
This is only used for testing. Let's change it as below:
```java
public void setCleanupTablesForTesting(Collection<String> tables) {
cleanupTables = tables == null || tables.isEmpty() ? Collections.emptySet()
: Collections.unmodifiableSet(new LinkedHashSet<>(tables));
}
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TableCacheUpdateTracker.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.db;
+
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.Set;
+
+/**
+ * Tracks table caches updated by the current thread.
+ */
+public final class TableCacheUpdateTracker {
+
+ private static final ThreadLocal<Tracker> CURRENT_TRACKER =
+ new ThreadLocal<>();
+ private static final Set<String> TRACKING = Collections.emptySet();
+
+ private TableCacheUpdateTracker() {
+ }
+
+ public static Tracker track() {
+ Tracker tracker = new Tracker(CURRENT_TRACKER.get());
+ CURRENT_TRACKER.set(tracker);
+ return tracker;
+ }
+
+ public static void recordCacheUpdate(String tableName) {
+ Tracker tracker = CURRENT_TRACKER.get();
+ if (tracker != null) {
+ tracker.recordCacheUpdate(tableName);
+ }
+ }
+
+ /**
+ * Tracks updated tables until the scope is closed.
+ */
+ public static final class Tracker implements AutoCloseable {
+ private final Tracker parent;
Review Comment:
Add a thread field and assertion:
```java
private final Thread thread = Thread.currentThread();
```
```java
//for all non-static methods, add
Preconditions.assertSame(thread, Thread.currentThread(), "thread");
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-5885518107
Thanks @szetszwo @adoroszlai for the reviews.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-4930777800
Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-5769838238
Hi @szetszwo , I've addressed the reviews. Could you help take a look when you have time?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-4627105326
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-5391378394
Thanks @szetszwo for the review, I will take revisit this when I have time.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-5885510204
Ok thanks @adoroszlai for the info. I'll go ahead and merge this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-5884563696
@adoroszlai Hi, it seems the kubernetes CI kept failing (https://github.com/apache/ozone/actions/runs/36218318505/job/109238015351?pr=10171) due to unable to download the artifact.
```
Downloading single artifact
Error: Unable to download artifact(s): Artifact not found for name: ozone-bin
Please ensure that your artifact is not expired and the artifact was uploaded using a compatible version of toolkit/upload-artifact.
For more information, visit the GitHub Artifacts FAQ: https://github.com/actions/toolkit/blob/main/packages/artifact/docs/faq.md
```
I tried restarting it multiple times, but to no avail. Is there a way to rectify this?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #10171: HDDS-13365. OM support automatic table cache tracking and cleanup
URL: https://github.com/apache/ozone/pull/10171
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #10171:
URL: https://github.com/apache/ozone/pull/10171#issuecomment-5693883375
Thanks @szetszwo for the review.
> My understanding is that each request may add something to the cache so the response should clean them up. Why the tracker need a parent?
Yes, the initial idea is that there is similar to Java `ScopedValue` and gRPC `Context.attach()` where we can have a nested scope by having a reference between the new (child) scope and the old (parent) scope in case when the parent scope has not been closed when creating a new scope. However, since OM Ratis applier only process one request at a time, then we don't need a nested scope. I've removed it.
> let's make the following changes the first patch:
Sure, let's reduce the scope for this patch first. The CleanupTableInfo mechanism is now reintroduced and only `OMOpenKeysDeleteRequest` and `OMOpenKeysDeleteResponse` is updated.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13365. OM support automatic table cache tracking and cleanup [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 merged PR #10171:
URL: https://github.com/apache/ozone/pull/10171
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org