-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[AMBARI-23032] Support for Blueprint exports for MPack-based clusters #716
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
[AMBARI-23032] Support for Blueprint exports for MPack-based clusters #716
Conversation
…nto AMBARI-23032-branch-feature-AMBARI-14714
…nto AMBARI-23032-branch-feature-AMBARI-14714
…nto AMBARI-23032-branch-feature-AMBARI-14714
…nto AMBARI-23032-branch-feature-AMBARI-14714
…nto AMBARI-23032-branch-feature-AMBARI-14714
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
rnettleton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this patch looks fine to me, just a minor issue in the javadocs that needs clarification.
One question: Is there a unit test that reproduces the NullPointerException that was occurring during Blueprint exports? If not, could you please add a unit test to this patch to verify that fix.
Thanks.
|
|
||
| /** | ||
| * @return the mpack associated with this component (can be {@code null} if component -> mpack mapping is unambiguous) | ||
| * @return the mpack associated with this component as {@link String} (can be {@code null} if component -> mpack mapping is unambiguous) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor issue in the javadocs:
Wouldn't this method return null if the mpack mapping was ambiguous, rather than unambiguous?
|
Hey @rnettleton, re unit test: actually it's a bit difficult. The cause of the issue was that the internals of AmbariContext changed. AmbariContext is normally created by Guice with its dependencies injected but ClusterBlueprintRenderer simply instantiated it (this was a wrong approach, but hadn't cause an error until the internals had changed and the code path was hitting an uninitialized dependency). I changed the implementation so that ClusterBlueprintRenderer uses a properly injected instance. Unfortunately writing a unit test for this would require setting up mocks for AmbariContext and the test would only prove whether mocks were properly set up. |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change works fine, except that the "Blueprints" key is not present in the result. This causes problem when trying to create a new cluster using the exported blueprint:
Creating security configuration from properties: null
NullPointerException
at org.apache.ambari.server.topology.SecurityConfigurationFactory.createSecurityConfigurationFromRequest(SecurityConfigurationFactory.java:76)
at org.apache.ambari.server.controller.internal.BlueprintResourceProvider$7.invoke(BlueprintResourceProvider.java:531)
|
Refer to this link for build results (access rights to CI server needed): |
|
@adoroszlai please check out the latest commit re exported "Blueprints" key. |
|
Refer to this link for build results (access rights to CI server needed): |
…nto AMBARI-23032-branch-feature-AMBARI-14714
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
What changes were proposed in this pull request?
Improvements for blueprint installation and export.
How was this patch tested?
Fixes were tested manually.
Unit test run results (updated):
Tests run: 4890, Failures: 27, Errors: 64, Skipped: 43 (this branch)
(the extra failing test: CredentialStoreTest.testInMemoryCredentialStoreService_CredentialExpired was successfully re-run)
vs
Tests run: 4890, Failures: 28, Errors: 63, Skipped: 43 (feature branch)