Skip to content

Conversation

@kaijchen
Copy link
Member

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.

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up this feature @kaijchen, and sorry I am so late on the review here. One minor inline comment but overall this looks good.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 TimeDurations for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kaijchen
Copy link
Member Author

Rebased to master to solve merge conflicts.

Copy link
Member

@captainzmc captainzmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Thanks @kaijchen for the patch. And thanks @errose28 for the review.

@captainzmc captainzmc merged commit 61a313f into apache:master Mar 22, 2022
@kaijchen
Copy link
Member Author

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.

@kaijchen kaijchen deleted the HDDS-6228 branch March 22, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants