Skip to content

Conversation

@adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Allow setting Configuration from strongly-typed config objects (ie. ones annotated with @Config). This is the opposite direction of existing ConfigurationSource.getObject().

One caveat is that the config object needs to be created via getObject() for default values from annotations to be properly set.

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

How was this patch tested?

Added unit test. Also updated a single part of "real" code where this change makes string constants unnecessary (DUFactory.Conf and TestDUFactory).

https://github.com/adoroszlai/hadoop-ozone/runs/795413474

@adoroszlai adoroszlai self-assigned this Jun 22, 2020
@adoroszlai adoroszlai requested a review from elek June 22, 2020 17:05

char[] getPassword(String key) throws IOException;

@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like the approach when the current port number (after using :0) is added to the configuration. It's easier if the configuration is immutable after loading.

I understand that the immutable is not possible with this injection and I accept that it's necessary.

One possible approach is to introduce two interfaces: ConfigSource (read-only) and ConfigDestination (write only). OzoneConfiguration can implement both. But there could be a specific ConfigSource (eg. EnvironmentVariableConfigSource) which would be read only.

But I am fine with committing this patch as is, but interested about opinion.

Also: it seems to be useful for testing to introduce an OzoneConfiguration.fromAnnotatedObject() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One possible approach is to introduce two interfaces: ConfigSource (read-only) and ConfigDestination (write only). OzoneConfiguration can implement both.

Thanks @elek for the suggestion. I was considering this, but needed this nudge to implement it.

Copy link
Member

Choose a reason for hiding this comment

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

I was considering this, but needed this nudge to implement it.

Oh, I see why. It makes the patch bigger because we use this dirty hack to store the port in the config.

Copy link
Member

@elek elek 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 the update, I didn't think it's such a big change (sorry, commenting is cheap and easy ;-) )

@elek elek merged commit 0300feb into apache:master Jul 1, 2020
@adoroszlai adoroszlai deleted the HDDS-2413 branch July 1, 2020 17:19
@adoroszlai
Copy link
Contributor Author

Thanks @elek for reviewing and committing it.

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