Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Feb 14, 2018

What changes were proposed in this pull request?

Fix the issue that the desired version of a cluster is not updated (only during cross-stack rolling upgrade).

How was this patch tested?

Running maven tests
Checked RU/EU and Ambari upgrade

@ghost ghost self-assigned this Feb 14, 2018
@ghost ghost requested review from jonathan-hurley and ncole February 14, 2018 15:28
Copy link
Contributor

@ncole ncole left a comment

Choose a reason for hiding this comment

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

Any unit tests covering this?

s_upgradeHelper.updateDesiredRepositoriesAndConfigs(upgradeContext);
if (direction == Direction.DOWNGRADE) {
StackId targetStack = upgradeContext.getCluster().getCurrentStackVersion();
cluster.setDesiredStackVersion(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you already have a StackId, no need to make a new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, did you happen to test an EU?

Copy link
Author

Choose a reason for hiding this comment

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

Tested EU, works well

Copy link
Member

@jonathan-hurley jonathan-hurley left a comment

Choose a reason for hiding this comment

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

You've made the changes here only for rolling and host ordered. Why does express not have this problem? Where is express setting the desired stack ID?

StackId currentStack = cluster.getCurrentStackVersion();
if (desiredStack.compareTo(currentStack) < 0) {
cluster.setDesiredStackVersion(currentStack);
}
Copy link
Member

Choose a reason for hiding this comment

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

If desired stack is > current stack, that's also a problem. Can you change this to a !equals() call instead?

Copy link
Author

@ghost ghost Feb 14, 2018

Choose a reason for hiding this comment

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

I assume that user is not expected to perform Ambari upgrade with a paused stack upgrade? Ok, will change the logic

Copy link
Author

Choose a reason for hiding this comment

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

@jonathan-hurley , regarding

Why does express not have this problem? 

EU updates cluster desired version during upgrade and downgrade at UpdateDesiredRepositoryAction. This action is called only from EU upgrade packs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's what I expected. However, I didn't see it in there. Can you identify where exactly it's set?

Copy link
Author

Choose a reason for hiding this comment

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

org/apache/ambari/server/serveraction/upgrades/UpdateDesiredRepositoryAction.java:149
at updateDesiredRepositoryVersion()

        // move the cluster's desired stack as well
        StackId targetStackId = targetRepositoryVersion.getStackId();
        cluster.setDesiredStackVersion(targetStackId);
      }

and below at the same file for downgrade.
EU was not broken, this code is already at 2.6 branch

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 looking in trunk :)

Thanks!

@ghost
Copy link
Author

ghost commented Feb 14, 2018

added few tests

@asfgit
Copy link

asfgit commented Feb 14, 2018

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

@asfgit
Copy link

asfgit commented Feb 15, 2018

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

@asfgit
Copy link

asfgit commented Feb 15, 2018

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

@ghost
Copy link
Author

ghost commented Feb 15, 2018

ci failure not related to changes, AMBARI-22918 broke it.

@ghost ghost merged commit e670279 into apache:branch-2.6 Feb 15, 2018
This pull request was closed.
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.

3 participants