Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HDDS-4309. Fix inconsistency recon config keys starting with recon and not ozone #1478

Merged
merged 4 commits into from
Oct 7, 2020
Merged

HDDS-4309. Fix inconsistency recon config keys starting with recon and not ozone #1478

merged 4 commits into from
Oct 7, 2020

Conversation

frischHWC
Copy link
Contributor

@frischHWC frischHWC commented Oct 6, 2020 •
edited by adoroszlai
Loading

What changes were proposed in this pull request?

Fix recon configs inconsistent

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4309

How was this patch tested?

Build up project, launch dockers, and new default parameters were present but not old ones anymore.

On recon container:

> ozone getconf confKey recon.om.socket.timeout
Configuration recon.om.socket.timeout is missing.
> ozone getconf confKey ozone.recon.om.socket.timeout
5s

@frischHWC frischHWC changed the title Fix inconsistenc recon config keys starting with recon and not ozone Fix inconsistency recon config keys starting with recon and not ozone Oct 6, 2020
vivekratnavel
vivekratnavel approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@vivekratnavel vivekratnavel left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@vivekratnavel
Copy link
Contributor

@frischHWC Thanks for working on this! The patch looks good to me except the failing checkstyles. Please fix those longer lines and I can merge it.

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconServerConfigKeys.java
 74: Line is longer than 80 characters (found 86).
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/OzoneManagerServiceProviderImpl.java
 128: Line is longer than 80 characters (found 82).
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/PrometheusServiceProviderImpl.java
 68: Line is longer than 80 characters (found 84).
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconWithOzoneManager.java
 99: Line is longer than 80 characters (found 82).
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestOzoneConfigurationFields.java
 72: Line is longer than 80 characters (found 82).

adoroszlai
adoroszlai requested changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @frischHWC for working on this.

The new key names look good, but if we simply change them, existing clusters would not respect after upgrade any custom value previously configured for these properties. Administrators would have to manually update their configs.

So I think we should keep the constants for the old names, and use them as fallback when looking up the config. For one of them (RECON_OM_SNAPSHOT_TASK_INITIAL_DELAY) I have suggested two changes, which can be applied to all other old config keys.

Note: commit message should include Jira ID (and match Jira title), eg. HDDS-4309. Fix inconsistent Recon config keys that start with "recon.om.". Whoever merges this, please pay attention to prepend Jira ID in commit message.

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconServerConfigKeys.java Show resolved Hide resolved
...on/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/OzoneManagerServiceProviderImpl.java Outdated
Comment on lines 208 to 209
OZONE_RECON_OM_SNAPSHOT_TASK_INITIAL_DELAY,
OZONE_RECON_OM_SNAPSHOT_TASK_INITIAL_DELAY_DEFAULT,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OZONE_RECON_OM_SNAPSHOT_TASK_INITIAL_DELAY,
OZONE_RECON_OM_SNAPSHOT_TASK_INITIAL_DELAY_DEFAULT,
OZONE_RECON_OM_SNAPSHOT_TASK_INITIAL_DELAY,
configuration.get(
ReconServerConfigKeys.RECON_OM_SNAPSHOT_TASK_INITIAL_DELAY,
OZONE_RECON_OM_SNAPSHOT_TASK_INITIAL_DELAY_DEFAULT),

@adoroszlai
Copy link
Contributor

Thanks @frischHWC for updating the patch. Sorry, I forgot TestOzoneConfigurationFields, a unit test that checks whether properties in ozone-default.xml and the *ConfigKeys classes are in sync. We need to enumerate the deprecated constants in addPropertiesNotInXml, since these are no longer present in the XML.

https://github.com/apache/hadoop-ozone/blob/f9b1ca45fdeaaa63b9355c5c54fafc96ddf4597e/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestOzoneConfigurationFields.java#L60-L76

@codecov-io
Copy link

Codecov Report

Merging #1478 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1478   +/-   ##
=========================================
  Coverage     75.49%   75.49%           
  Complexity    10798    10798           
=========================================
  Files          1021     1021           
  Lines         52095    52095           
  Branches       5103     5103           
=========================================
  Hits          39328    39328           
  Misses        10321    10321           
  Partials       2446     2446

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9b1ca4...a2bd180. Read the comment docs.

@vivekratnavel vivekratnavel merged commit efaa4fc into apache:master Oct 7, 2020
@vivekratnavel vivekratnavel changed the title Fix inconsistency recon config keys starting with recon and not ozone HDDS-4309. Fix inconsistency recon config keys starting with recon and not ozone Oct 7, 2020
@vivekratnavel
Copy link
Contributor

@adoroszlai Thanks for the review and @frischHWC thanks for the contribution.

errose28 added a commit to errose28/ozone that referenced this pull request Oct 14, 2020
@errose28
Merge branch 'master' into HDDS-4123
7100ff7 
* master: (23 commits)
  HDDS-4122. Implement OM Delete Expired Open Key Request and Response (apache#1435)
  HDDS-4336. ContainerInfo does not persist BCSID (sequenceId) leading to failed replica reports (apache#1488)
  Remove extra serialization from getBlockID (apache#1470)
  HDDS-4262. Use ClientID and CallID from Rpc Client to detect retry requests (apache#1436)
  HDDS-4285. Read is slow due to frequent calls to UGI.getCurrentUser() and getTokens() (apache#1454)
  HDDS-4312. findbugs check succeeds despite compile error (apache#1476)
  HDDS-4311. Type-safe config design doc points to OM HA (apache#1477)
  HDDS-3814. Drop a column family through debug cli tool (apache#1083)
  HDDS-3728. Bucket space: check quotaUsageInBytes when write key and allocate block. (apache#1458)
  HDDS-4316. Upgrade to angular 1.8.0 due to CVE-2020-7676 (apache#1481)
  HDDS-4325. Incompatible return codes from Ozone getconf -confKey (apache#1485). Contributed by Doroszlai, Attila.
  HDDS-4309. Fix inconsistency in recon config keys starting with recon and not ozone (apache#1478)
  HDDS-4310: Ozone getconf broke the compatibility (apache#1475)
  HDDS-4298. Use an interface in Ozone client instead of XceiverClientManager (apache#1460)
  HDDS-4280. Document notable configurations for Recon. (apache#1448)
  HDDS-4156. add hierarchical layout to Chinese doc (apache#1368)
  HDDS-4242. Copy PrefixInfo proto to new project hadoop-ozone/interface-storage (apache#1444)
  HDDS-4264. Uniform naming conventions of Ozone Shell Options. (apache#1447)
  HDDS-4271. Avoid logging chunk content in Ozone Insight (apache#1466)
  HDDS-4299. Display Ratis version with ozone version (apache#1464)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vivekratnavel vivekratnavel vivekratnavel approved these changes

@adoroszlai adoroszlai adoroszlai approved these changes

@avijayanhwx avijayanhwx Awaiting requested review from avijayanhwx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@frischHWC @vivekratnavel @adoroszlai @codecov-io