[AMBARI-22668] Moving LDAP related properties to DB upon upgrade to 3.0.0 #106

smolnar82 · 2018-01-12T13:13:58Z

@rlevas @zeroflag @echekanskiy Please review

What changes were proposed in this pull request?

According to the requirement I needed to move LDAP related properties out from ambari.properties file into our DB table called ambari_configuration with 'ldap-configuration' category name when upgrading to 3.0.0.

How was this patch tested?

Unit test were added and adopted; local build result:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 02:37 h
[INFO] Finished at: 2018-01-12T13:40:34+01:00
[INFO] Final Memory: 207M/802M
[INFO] ------------------------------------------------------------------------

Besides unit testing the following integration test has been executed:

installed Ambari 2.6.2
executed ambari-server setup-ldap (LDAP related properties appeared in /etc/ambari-server/conf/ambari.properties; made a backup of this file)
upgraded to Ambari 3.0.0 (using build 1426 and replaced ambari-server.jar to include my changes): Ambari Server 'upgrade' completed successfully.
DB check: all properties I defined in step 2 have been added in ambari_configration table with 'ldap-configuration' category
Property file check: LDAP related properties have been removed

zeroflag · 2018-01-12T13:33:52Z

ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java

maybe anyMatch() would be shorter

This implementation also removes properties with prefix and/or suffix, eg. security.server.crt_pass would match security.server.crt_pass_file, too, due to using indexOf(). It may not be a problem in this use case, but I think it should be addressed, since it is a generic method.

adoroszlai · 2018-01-12T14:16:45Z

ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java

This implementation also removes properties with prefix and/or suffix, eg. security.server.crt_pass would match security.server.crt_pass_file, too, due to using indexOf(). It may not be a problem in this use case, but I think it should be addressed, since it is a generic method.

adoroszlai · 2018-01-12T14:18:04Z

ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog300.java

typo in ambari_congiuration

adoroszlai · 2018-01-12T14:19:13Z

ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java

Deleting the file before writing the new one may lead to data loss.

asfgit · 2018-01-13T00:30:10Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/83/
Test FAILed.
Test FAILured.

adoroszlai

Thanks for the changes.

One more request: please do not force-push to the source branch of pull requests, since it makes it harder to see what previous comments were about (and how the comments were addressed). Instead, please push additional commits. They can be squashed during merge if a single commit on the target branch is desired.

adoroszlai · 2018-01-13T17:29:30Z

ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog300.java

Upgrade logic should be idempotent:

Running upgrade on already upgraded setup should be no-op (no errors etc.). I guess this is the case, since properties removed at the end will not be processed on next try.

Retrying partially completed upgrade should resume without errors. This might be a problem here, since all properties are left until the end, duplicate properties may be created. Can you please confirm if this handled correctly? (Test by throwing some exception in the middle of property processing.) If not, can you please make sure to "create or update" the properties instead of "create".

These are good notices; thanks for bringing them up.
Point 1: like you indicated there won't be more properties to be upgraded = no-op
Point 2: I'll check it out and modify the code if needed

rlevas · 2018-01-13T22:15:53Z

ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java

You should use org.apache.ambari.server.configuration.Configuration#readConfigFile to read the properties from the ambari.properties file into a java.util.Properties object. Then later write out the properties using org.apache.ambari.server.configuration.Configuration#writeConfigFile (which does not yet exist).

rlevas · 2018-01-13T22:20:28Z

ambari-server/src/main/java/org/apache/ambari/server/utils/HostAndPort.java

Can java.net.URL be used instead?

Using HostAndPort from Guava instead

rlevas · 2018-01-13T22:20:53Z

ambari-server/src/main/java/org/apache/ambari/server/utils/HostAndPort.java

Can java.net.URL be used instead?

Using HostAndPort from Guava instead

asfgit · 2018-01-15T00:13:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/116/
Test FAILed.
Test FAILured.

asfgit · 2018-01-15T10:37:59Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/120/
Test FAILed.
Test FAILured.

asfgit · 2018-01-15T13:18:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/124/
Test FAILed.
Test FAILured.

smolnar82 · 2018-01-15T14:33:47Z

Removed my commit which fixed the JUnit tests since @adoroszlai fixed it in #118

asfgit · 2018-01-15T18:11:08Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/132/
Test PASSed.

smolnar82 · 2018-01-15T18:21:15Z

Tests have been passed. Could someone with the proper privileges merge this PR?

zeroflag reviewed Jan 12, 2018

View reviewed changes

adoroszlai suggested changes Jan 12, 2018

View reviewed changes

smolnar82 force-pushed the AMBARI-22668 branch from 1da6105 to 581059a Compare January 12, 2018 22:35

adoroszlai approved these changes Jan 13, 2018

View reviewed changes

adoroszlai reviewed Jan 13, 2018

View reviewed changes

rlevas suggested changes Jan 13, 2018

View reviewed changes

smolnar82 force-pushed the AMBARI-22668 branch from 0a8c52b to a1cd1a0 Compare January 15, 2018 08:59

zeroflag approved these changes Jan 15, 2018

View reviewed changes

rlevas approved these changes Jan 15, 2018

View reviewed changes

Sandor Molnar added 3 commits January 15, 2018 15:31

AMBARI-22668. Moving LDAP related properties to DB upon upgrade to 3.0.0

b2a61ec

AMBARI-22668: implemented changes requested by reviewers

0c61412

AMBARI-22668: implemented changes requested by rlevas

1e5d726

smolnar82 force-pushed the AMBARI-22668 branch from 0508d66 to 1e5d726 Compare January 15, 2018 14:32

rlevas changed the title ~~AMBARI-22668. Moving LDAP related properties to DB upon upgrade to 3.0.0~~ [AMBARI-22668] Moving LDAP related properties to DB upon upgrade to 3.0.0 Jan 15, 2018

rlevas merged commit 23e51fa into apache:trunk Jan 15, 2018

smolnar82 deleted the AMBARI-22668 branch January 15, 2018 20:00

[AMBARI-22668] Moving LDAP related properties to DB upon upgrade to 3.0.0 #106

[AMBARI-22668] Moving LDAP related properties to DB upon upgrade to 3.0.0 #106

Uh oh!

Conversation

smolnar82 commented Jan 12, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Jan 13, 2018

Uh oh!

adoroszlai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Jan 15, 2018

Uh oh!

asfgit commented Jan 15, 2018

Uh oh!

asfgit commented Jan 15, 2018

Uh oh!

smolnar82 commented Jan 15, 2018

Uh oh!

asfgit commented Jan 15, 2018

Uh oh!

smolnar82 commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adoroszlai left a comment •

edited

Loading