Skip to content

HDDS-2661. Use field based Config annotation instead of method based - #297

Closed
elek wants to merge 5 commits into
apache:masterfrom
elek:HDDS-2661
Closed

HDDS-2661. Use field based Config annotation instead of method based#297
elek wants to merge 5 commits into
apache:masterfrom
elek:HDDS-2661

Conversation

@elek

@elek elek commented Dec 4, 2019

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

HDDS-2413 proposes an additional usage of the @config annotation: to set configuration based on an existing configuration class.

But as of now we annotate the setters instead of the fields. To avoid annotation duplication (we need to read the values from the getters or the fields) I propose to switch to use field based annotations instead of setter based annotation.

I think it's more readable and additional validation (even the class level validations) can be done in a @PostConstruct method.

How is it implemented

To support the annotation in private field we need to update our logic to check the fields of the parent class(es). It was not necessary with the setter based approach as the setter methods are public inherited. The patch updates both the annoation processor and the reflection based API (OzoneConfiguration).

With field based annotation injection it's not possible to put validation logic to the setters, therefor the standard @PostConstruct annotation is used to provide class level validation (or any other execution).

What is the link to the Apache JIRA

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

How was this patch tested?

Mainly with unit tests. New unit test is introduced in configuration project and the TestOzoneConfiguration is also improved.

@adoroszlai adoroszlai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @elek for implementing this. I have verified that generated default configs match those from master (with some reordering). I would love if complexity of getObject could be reduced, but it's fine if it will be done only in one of the follow-up changes.

@elek

elek commented Dec 5, 2019

Copy link
Copy Markdown
Member Author

I would love if complexity of getObject could be reduced...

Sure, thanks the review / feedback. I moved a few steps to separated methods to improve the readability (like calling the @PostConstruct annotated method) but it's not a full solution. Let me know if you have more powerful idea and we can do it in follow up steps.

As a next step I will implement HDDS-2413 (the opposite direction for unit testing...(

@adoroszlai

Copy link
Copy Markdown
Contributor

I moved a few steps to separated methods to improve the readability (like calling the @PostConstruct annotated method)

Thank you very much for improving it.

@elek
elek requested a review from arp7 December 12, 2019 14:01
@elek

elek commented Jan 6, 2020

Copy link
Copy Markdown
Member Author

Thanks the review @adoroszlai. I will commit it after a new CI build (Just to me sure if it's still stable).

@adoroszlai

Copy link
Copy Markdown
Contributor

Thanks @elek for the new CI run, looks good.

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.

2 participants