You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/01/27 07:27:28 UTC

[GitHub] [ozone] kaijchen opened a new pull request #3022: HDDS-6228. Update config keys for open key cleanup service

kaijchen opened a new pull request #3022:
URL: https://github.com/apache/ozone/pull/3022


   ## What changes were proposed in this pull request?
   
   Move config keys for open key cleanup service from OzoneConfigKeys to OMConfigKeys. 
   Change key names and default values as the design doc.
   
   This change was part of #1511. The differences are:
   
   1. `org.apache.ratis.util.TimeDuration` was replaced by `java.time.Duration`.
   2. `Instant.now()` is only called once per query in `getExpiredOpenKeys()`.
   
   Since the service needs refinement such as Quota and FSO support,
   let's update the config keys first. 
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/HDDS-6228
   
   ## How was this patch tested?
   
   Integration tests.
   


-- 
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


[GitHub] [ozone] kaijchen commented on pull request #3022: HDDS-6228. Update config keys for open key cleanup service

Posted by GitBox <gi...@apache.org>.
kaijchen commented on pull request #3022:
URL: https://github.com/apache/ozone/pull/3022#issuecomment-1074961135


   Thanks @errose28 and @captainzmc for the review.
   
   > It does not look like the tests that were setting open key cleanup actually need the config, so maybe we can remove those settings in this PR too.
   
   Sorry I forgot to address this, I will fix it when implementing HDDS-4123. 


-- 
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


[GitHub] [ozone] kaijchen commented on a change in pull request #3022: HDDS-6228. Update config keys for open key cleanup service

Posted by GitBox <gi...@apache.org>.
kaijchen commented on a change in pull request #3022:
URL: https://github.com/apache/ozone/pull/3022#discussion_r831826488



##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
##########
@@ -83,6 +84,22 @@ private OMConfigKeys() {
       "ozone.key.deleting.limit.per.task";
   public static final int OZONE_KEY_DELETING_LIMIT_PER_TASK_DEFAULT = 20000;
 
+  public static final String OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL =
+      "ozone.om.open.key.cleanup.service.interval";
+  public static final Duration
+      OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL_DEFAULT =
+      Duration.ofHours(24);
+
+  public static final String OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD =
+      "ozone.om.open.key.expire.threshold";
+  public static final Duration OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD_DEFAULT =

Review comment:
       Yes, String is indeed better. And it's also the same representation as in `ozone-default.xml`. 




-- 
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


[GitHub] [ozone] errose28 commented on a change in pull request #3022: HDDS-6228. Update config keys for open key cleanup service

Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #3022:
URL: https://github.com/apache/ozone/pull/3022#discussion_r829348902



##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
##########
@@ -83,6 +84,22 @@ private OMConfigKeys() {
       "ozone.key.deleting.limit.per.task";
   public static final int OZONE_KEY_DELETING_LIMIT_PER_TASK_DEFAULT = 20000;
 
+  public static final String OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL =
+      "ozone.om.open.key.cleanup.service.interval";
+  public static final Duration
+      OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL_DEFAULT =
+      Duration.ofHours(24);
+
+  public static final String OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD =
+      "ozone.om.open.key.expire.threshold";
+  public static final Duration OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD_DEFAULT =

Review comment:
       I think the Ratis `TimeDuration` was used since existing time configs in this class and the other config key classes use it. I am not sure if it provides any benefits over the Java `Duration` class, but I think we should keep these as `TimeDuration`s for consistency.




-- 
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


[GitHub] [ozone] kaijchen commented on a change in pull request #3022: HDDS-6228. Update config keys for open key cleanup service

Posted by GitBox <gi...@apache.org>.
kaijchen commented on a change in pull request #3022:
URL: https://github.com/apache/ozone/pull/3022#discussion_r829624680



##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
##########
@@ -83,6 +84,22 @@ private OMConfigKeys() {
       "ozone.key.deleting.limit.per.task";
   public static final int OZONE_KEY_DELETING_LIMIT_PER_TASK_DEFAULT = 20000;
 
+  public static final String OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL =
+      "ozone.om.open.key.cleanup.service.interval";
+  public static final Duration
+      OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL_DEFAULT =
+      Duration.ofHours(24);
+
+  public static final String OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD =
+      "ozone.om.open.key.expire.threshold";
+  public static final Duration OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD_DEFAULT =

Review comment:
       Every other Ratis `TimeDuration` in this class is related to Ratis.
   Either it starts with `ozone.om.ratis.server` or `ozone.om.snapshot`.
   I changed this to java `Duration` because I think this config is not related to Ratis.
   Do you think we still need to keep it as `TimeDuration`?




-- 
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


[GitHub] [ozone] errose28 commented on a change in pull request #3022: HDDS-6228. Update config keys for open key cleanup service

Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #3022:
URL: https://github.com/apache/ozone/pull/3022#discussion_r831474866



##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
##########
@@ -83,6 +84,22 @@ private OMConfigKeys() {
       "ozone.key.deleting.limit.per.task";
   public static final int OZONE_KEY_DELETING_LIMIT_PER_TASK_DEFAULT = 20000;
 
+  public static final String OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL =
+      "ozone.om.open.key.cleanup.service.interval";
+  public static final Duration
+      OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL_DEFAULT =
+      Duration.ofHours(24);
+
+  public static final String OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD =
+      "ozone.om.open.key.expire.threshold";
+  public static final Duration OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD_DEFAULT =

Review comment:
       That's a good point. Actually it looks like most existing intervals are either configured as Strings with suffixes or as longs. If we want to be matching I think Strings would be the best bet, but Duration is fine too.




-- 
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


[GitHub] [ozone] captainzmc merged pull request #3022: HDDS-6228. Update config keys for open key cleanup service

Posted by GitBox <gi...@apache.org>.
captainzmc merged pull request #3022:
URL: https://github.com/apache/ozone/pull/3022


   


-- 
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


[GitHub] [ozone] errose28 commented on a change in pull request #3022: HDDS-6228. Update config keys for open key cleanup service

Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #3022:
URL: https://github.com/apache/ozone/pull/3022#discussion_r829348902



##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
##########
@@ -83,6 +84,22 @@ private OMConfigKeys() {
       "ozone.key.deleting.limit.per.task";
   public static final int OZONE_KEY_DELETING_LIMIT_PER_TASK_DEFAULT = 20000;
 
+  public static final String OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL =
+      "ozone.om.open.key.cleanup.service.interval";
+  public static final Duration
+      OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL_DEFAULT =
+      Duration.ofHours(24);
+
+  public static final String OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD =
+      "ozone.om.open.key.expire.threshold";
+  public static final Duration OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD_DEFAULT =

Review comment:
       I think the Ratis `TimeDuration` was used since existing time configs in this class and the other config key classes use it. I am not sure if it provides any benefits over the Java `Duration` class, but I think we should keep these as `TimeDuration`s for consistency.




-- 
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


[GitHub] [ozone] kaijchen commented on a change in pull request #3022: HDDS-6228. Update config keys for open key cleanup service

Posted by GitBox <gi...@apache.org>.
kaijchen commented on a change in pull request #3022:
URL: https://github.com/apache/ozone/pull/3022#discussion_r829624680



##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
##########
@@ -83,6 +84,22 @@ private OMConfigKeys() {
       "ozone.key.deleting.limit.per.task";
   public static final int OZONE_KEY_DELETING_LIMIT_PER_TASK_DEFAULT = 20000;
 
+  public static final String OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL =
+      "ozone.om.open.key.cleanup.service.interval";
+  public static final Duration
+      OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL_DEFAULT =
+      Duration.ofHours(24);
+
+  public static final String OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD =
+      "ozone.om.open.key.expire.threshold";
+  public static final Duration OZONE_OM_OPEN_KEY_EXPIRE_THRESHOLD_DEFAULT =

Review comment:
       Every other Ratis `TimeDuration` in this class is related to Ratis.
   Either it starts with `ozone.om.ratis.server` or `ozone.om.snapshot`.
   I changed this to java `Duration` because I think this config is not related to Ratis.
   Do you think we still need to keep it as `TimeDuration`?




-- 
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


[GitHub] [ozone] kaijchen commented on pull request #3022: HDDS-6228. Update config keys for open key cleanup service

Posted by GitBox <gi...@apache.org>.
kaijchen commented on pull request #3022:
URL: https://github.com/apache/ozone/pull/3022#issuecomment-1073520266


   Rebased to master to solve merge conflicts.


-- 
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