You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "smengcl (via GitHub)" <gi...@apache.org> on 2025/07/17 22:50:40 UTC
[PR] HDDS-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
smengcl opened a new pull request, #8825:
URL: https://github.com/apache/ozone/pull/8825
## What changes were proposed in this pull request?
This patch makes `ozone.snapshot.filtering.service.interval` reconfigurable to allow live enabling/disabling/restarting SST filtering service in addition to service interval reconfiguration.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13464
## How was this patch tested?
- Test case to be added
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #8825:
URL: https://github.com/apache/ozone/pull/8825#issuecomment-3090702404
> Loos good to me. But why make it reconfigurble?
This would give us flexibility to disable/enable sst filtering service without having to restart OM. Through a workaround we can use the snapshot feature to get a consistent point in time view of the om rocksdb when sst filtering service is disabled.
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8825:
URL: https://github.com/apache/ozone/pull/8825#issuecomment-3103896867
Thanks @smengcl for updating the patch.
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #8825:
URL: https://github.com/apache/ozone/pull/8825#discussion_r2219532471
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -257,6 +257,7 @@ public BootstrapStateHandler.Lock getBootstrapStateLock() {
@Override
public void shutdown() {
+ LOG.debug("Shutting down SstFilteringService");
Review Comment:
done
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #8825:
URL: https://github.com/apache/ozone/pull/8825#issuecomment-3103792357
> Thanks @smengcl for the patch.
Thanks @adoroszlai ! I have address your comments.
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #8825:
URL: https://github.com/apache/ozone/pull/8825#issuecomment-3104358455
Thanks @jojochuang @swamirishi @adoroszlai for the reviews and comments.
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8825:
URL: https://github.com/apache/ozone/pull/8825#discussion_r2216908325
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -257,6 +257,7 @@ public BootstrapStateHandler.Lock getBootstrapStateLock() {
@Override
public void shutdown() {
+ LOG.debug("Shutting down SstFilteringService");
Review Comment:
Can we make this info level log
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl merged PR #8825:
URL: https://github.com/apache/ozone/pull/8825
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #8825:
URL: https://github.com/apache/ozone/pull/8825#issuecomment-3090723917
Ok please update jira. Right now it's blank empty.
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #8825:
URL: https://github.com/apache/ozone/pull/8825#discussion_r2220861094
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -257,6 +257,7 @@ public BootstrapStateHandler.Lock getBootstrapStateLock() {
@Override
public void shutdown() {
+ LOG.debug("Shutting down SstFilteringService");
Review Comment:
I realized the super `BackgroundService` already includes INFO level service start/stop message, thus I am removing this new log print. See the log attached to the test section for details.
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8825:
URL: https://github.com/apache/ozone/pull/8825#discussion_r2221073348
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -370,6 +355,41 @@ public void start(OzoneConfiguration configuration) {
: new CachedDNSToSwitchMapping(newInstance));
}
+ /**
+ * Start the snapshot SST filtering service if interval is not set to disabled value.
+ * @param conf
+ */
+ public void startSnapshotSstFilteringService(OzoneConfiguration conf) {
+ long serviceInterval = conf.getTimeDuration(
+ OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL,
+ OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ long serviceTimeout = conf.getTimeDuration(
+ OZONE_SNAPSHOT_SST_FILTERING_SERVICE_TIMEOUT,
+ OZONE_SNAPSHOT_SST_FILTERING_SERVICE_TIMEOUT_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ if (isSstFilteringSvcEnabled()) {
+ snapshotSstFilteringService =
+ new SstFilteringService(serviceInterval, TimeUnit.MILLISECONDS,
+ serviceTimeout, ozoneManager, conf);
Review Comment:
nit: can move conf lookup inside `if`
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -5206,6 +5208,26 @@ private String reconfOzoneThreadNumberDirDeletion(String newVal) {
return newVal;
}
+ private String reconfFSSnapshotSSTFilteringServiceInterval(String newVal) {
+ boolean wasSstFilteringSvcEnabled = ((KeyManagerImpl) keyManager).isSstFilteringSvcEnabled();
+ getConfiguration().set(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL, newVal);
+
+ if (this.isFilesystemSnapshotEnabled()) {
+ if (wasSstFilteringSvcEnabled) {
+ // Sanity check
+ Preconditions.checkNotNull(keyManager.getSnapshotSstFilteringService(),
+ "sstFilteringService should not be null when SST filtering service is enabled.");
+ ((KeyManagerImpl) keyManager).stopSnapshotSstFilteringService();
Review Comment:
Can we change `keyManager` declaration to `KeyManagerImpl`?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestOmReconfiguration.java:
##########
@@ -125,4 +131,59 @@ void unsetAllowListAllVolumes(String newValue) throws ReconfigurationException {
assertEquals(OZONE_OM_VOLUME_LISTALL_ALLOWED_DEFAULT, cluster().getOzoneManager().getAllowListAllVolumes());
}
+ @Test
+ void sstFilteringServiceInterval() throws ReconfigurationException {
+ // Tests reconfiguration of SST filtering service interval
+
+ // Get the original interval value
+ String originalValue = cluster().getOzoneManager().getConfiguration()
+ .get(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL);
+ // Verify the original value is valid (should be larger than -1)
+ long originalInterval = cluster().getOzoneManager().getConfiguration()
+ .getTimeDuration(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL,
+ org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL_DEFAULT,
+ java.util.concurrent.TimeUnit.MILLISECONDS);
+ assertTrue(originalInterval > 0, "Original interval should be greater than 0");
Review Comment:
nit: for failure message that includes unexpected actual value, prefer `assertThat(originalInterval).isGreaterThan(...)`, or even `assertThat(originalInterval).isPositive()` in this case.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestOmReconfiguration.java:
##########
@@ -125,4 +131,59 @@ void unsetAllowListAllVolumes(String newValue) throws ReconfigurationException {
assertEquals(OZONE_OM_VOLUME_LISTALL_ALLOWED_DEFAULT, cluster().getOzoneManager().getAllowListAllVolumes());
}
+ @Test
+ void sstFilteringServiceInterval() throws ReconfigurationException {
+ // Tests reconfiguration of SST filtering service interval
+
+ // Get the original interval value
+ String originalValue = cluster().getOzoneManager().getConfiguration()
+ .get(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL);
+ // Verify the original value is valid (should be larger than -1)
+ long originalInterval = cluster().getOzoneManager().getConfiguration()
+ .getTimeDuration(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL,
+ org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL_DEFAULT,
+ java.util.concurrent.TimeUnit.MILLISECONDS);
+ assertTrue(originalInterval > 0, "Original interval should be greater than 0");
+
+ // 1. Test reconfiguring to a different valid interval (30 seconds)
+ // This should restart the SstFilteringService
+ final String newIntervalValue = "30s";
+ getSubject().reconfigurePropertyImpl(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL, newIntervalValue);
+ assertEquals(newIntervalValue, cluster().getOzoneManager().getConfiguration()
+ .get(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL));
+ // Verify the service is still enabled with the new interval
+ assertTrue(((org.apache.hadoop.ozone.om.KeyManagerImpl) cluster().getOzoneManager().getKeyManager())
Review Comment:
nit: store `keyManager` in local variable to reduce clutter
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -5206,6 +5208,26 @@ private String reconfOzoneThreadNumberDirDeletion(String newVal) {
return newVal;
}
+ private String reconfFSSnapshotSSTFilteringServiceInterval(String newVal) {
Review Comment:
nit: please remove `FS` from the name (`reconfFS` reads hard, and it's not part of the config key anyway)
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -527,7 +528,8 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption)
.register(OZONE_KEY_DELETING_LIMIT_PER_TASK,
this::reconfOzoneKeyDeletingLimitPerTask)
.register(OZONE_DIR_DELETING_SERVICE_INTERVAL, this::reconfOzoneDirDeletingServiceInterval)
- .register(OZONE_THREAD_NUMBER_DIR_DELETION, this::reconfOzoneThreadNumberDirDeletion);
+ .register(OZONE_THREAD_NUMBER_DIR_DELETION, this::reconfOzoneThreadNumberDirDeletion)
+ .register(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL, this::reconfFSSnapshotSSTFilteringServiceInterval);
Review Comment:
nit: could reduce diff by adding it before the last call
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestOmReconfiguration.java:
##########
@@ -125,4 +131,59 @@ void unsetAllowListAllVolumes(String newValue) throws ReconfigurationException {
assertEquals(OZONE_OM_VOLUME_LISTALL_ALLOWED_DEFAULT, cluster().getOzoneManager().getAllowListAllVolumes());
}
+ @Test
+ void sstFilteringServiceInterval() throws ReconfigurationException {
+ // Tests reconfiguration of SST filtering service interval
+
+ // Get the original interval value
+ String originalValue = cluster().getOzoneManager().getConfiguration()
+ .get(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL);
+ // Verify the original value is valid (should be larger than -1)
+ long originalInterval = cluster().getOzoneManager().getConfiguration()
+ .getTimeDuration(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL,
+ org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL_DEFAULT,
+ java.util.concurrent.TimeUnit.MILLISECONDS);
Review Comment:
nit: import `MILLISECONDS`
--
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-13464. Make ozone.snapshot.filtering.service.interval reconfigurable [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #8825:
URL: https://github.com/apache/ozone/pull/8825#discussion_r2223160248
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestOmReconfiguration.java:
##########
@@ -125,4 +131,59 @@ void unsetAllowListAllVolumes(String newValue) throws ReconfigurationException {
assertEquals(OZONE_OM_VOLUME_LISTALL_ALLOWED_DEFAULT, cluster().getOzoneManager().getAllowListAllVolumes());
}
+ @Test
+ void sstFilteringServiceInterval() throws ReconfigurationException {
+ // Tests reconfiguration of SST filtering service interval
+
+ // Get the original interval value
+ String originalValue = cluster().getOzoneManager().getConfiguration()
+ .get(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL);
+ // Verify the original value is valid (should be larger than -1)
+ long originalInterval = cluster().getOzoneManager().getConfiguration()
+ .getTimeDuration(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL,
+ org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL_DEFAULT,
+ java.util.concurrent.TimeUnit.MILLISECONDS);
+ assertTrue(originalInterval > 0, "Original interval should be greater than 0");
Review Comment:
Done. Thanks for the suggestion!
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestOmReconfiguration.java:
##########
@@ -125,4 +131,59 @@ void unsetAllowListAllVolumes(String newValue) throws ReconfigurationException {
assertEquals(OZONE_OM_VOLUME_LISTALL_ALLOWED_DEFAULT, cluster().getOzoneManager().getAllowListAllVolumes());
}
+ @Test
+ void sstFilteringServiceInterval() throws ReconfigurationException {
+ // Tests reconfiguration of SST filtering service interval
+
+ // Get the original interval value
+ String originalValue = cluster().getOzoneManager().getConfiguration()
+ .get(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL);
+ // Verify the original value is valid (should be larger than -1)
+ long originalInterval = cluster().getOzoneManager().getConfiguration()
+ .getTimeDuration(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL,
+ org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL_DEFAULT,
+ java.util.concurrent.TimeUnit.MILLISECONDS);
+ assertTrue(originalInterval > 0, "Original interval should be greater than 0");
+
+ // 1. Test reconfiguring to a different valid interval (30 seconds)
+ // This should restart the SstFilteringService
+ final String newIntervalValue = "30s";
+ getSubject().reconfigurePropertyImpl(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL, newIntervalValue);
+ assertEquals(newIntervalValue, cluster().getOzoneManager().getConfiguration()
+ .get(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL));
+ // Verify the service is still enabled with the new interval
+ assertTrue(((org.apache.hadoop.ozone.om.KeyManagerImpl) cluster().getOzoneManager().getKeyManager())
Review Comment:
done
--
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