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.

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

HDDS-8702. Extract handler for reconfigurable properties #4794

Merged
merged 9 commits into from
Jun 7, 2023

Conversation

adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Some logic related to reconfiguration can be refactored into a common handler object to reduce the complexity and avoid duplication.

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

How was this patch tested?

Added unit test, refactored existing integration test.

Tested CLI commands in ozone compose environment.

$ docker-compose exec om bash
bash-4.2$ ozone admin reconfig --address om:9862 properties
OM: Node [om:9862] Reconfigurable properties:
ozone.administrators
ozone.readonly.administrators
bash-4.2$ vi /etc/hadoop/ozone-site.xml
bash-4.2$ ozone admin reconfig --address om:9862 start
OM: Started reconfiguration task on node [om:9862].
bash-4.2$ ozone admin reconfig --address om:9862 status
OM: Reconfiguring status for node [om:9862]: started at Thu May 25 15:26:05 UTC 2023 and finished at Thu May 25 15:26:05 UTC 2023.
SUCCESS: Changed property ozone.administrators
	From: ""
	To: "foo"
bash-4.2$ exit

$ docker-compose exec scm bash                   
bash-4.2$ ozone admin reconfig --address scm:9860 properties
SCM: Node [scm:9860] Reconfigurable properties:
ozone.administrators
bash-4.2$ vi /etc/hadoop/ozone-site.xml
bash-4.2$ ozone admin reconfig --address scm:9860 start
SCM: Started reconfiguration task on node [scm:9860].
bash-4.2$ ozone admin reconfig --address scm:9860 status
SCM: Reconfiguring status for node [scm:9860]: started at Thu May 25 15:33:59 UTC 2023 and finished at Thu May 25 15:33:59 UTC 2023.
SUCCESS: Changed property ozone.administrators
	From: ""
	To: "bar"
bash-4.2$ exit

https://github.com/adoroszlai/hadoop-ozone/actions/runs/5117942592

@adoroszlai adoroszlai self-assigned this May 30, 2023
@adoroszlai
Copy link
Contributor Author

@whbing @xichen01 please take a look

Copy link
Contributor

@xichen01 xichen01 left a comment

Choose a reason for hiding this comment

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

LGTM. Just a little commented you can refer to

@Override
public String reconfigurePropertyImpl(String property, String newValue) {
return properties.getOrDefault(property, identity())
.apply(newValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we only return the newValue String when the reconfig is execute normal, If the request property is not contain in the properties, we can throw an exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we only return the newValue String when the reconfig is execute normal, If the request property is not contain in the properties, we can throw an exception.

Now do nothing if the request property is not contain in the properties. We can see the result by exe status.

@whbing
Copy link
Contributor

whbing commented May 31, 2023

Thanks @adoroszlai for providing an elegant refactoring. I tested the PR in compose environment again and the result is as expected.
LGTM. +1

@adoroszlai adoroszlai marked this pull request as draft May 31, 2023 16:27
@adoroszlai
Copy link
Contributor Author

Thanks @whbing, @xichen01 for the review. I have updated the patch based on your suggestion (which seems to have been deleted) to also implement ReconfigureProtocol in the handler. Good idea, it helps further reduce duplication. We can also get rid of multiple copies of getRemoteUser() function.

CI is still running in my fork, will mark this ready for review if it succeeds.

@adoroszlai adoroszlai marked this pull request as ready for review May 31, 2023 18:01
@adoroszlai adoroszlai marked this pull request as draft June 2, 2023 02:34
@adoroszlai adoroszlai marked this pull request as ready for review June 2, 2023 11:07
@whbing
Copy link
Contributor

whbing commented Jun 4, 2023

+1

@adoroszlai
Copy link
Contributor Author

@captainzmc @dombizita please review

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.

Thanks @adoroszlai for the patch. The refactoring looks good. Let's merge this.

@captainzmc captainzmc merged commit ed029c9 into apache:master Jun 7, 2023
36 checks passed
@adoroszlai adoroszlai deleted the HDDS-8702 branch June 7, 2023 06:20
@adoroszlai
Copy link
Contributor Author

Thanks @captainzmc, @whbing, @xichen01 for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants