Skip to content
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.

Sign up for GitHub

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

Jump to bottom

STORM-2863: Reset NormalizedResources for tests #2477

Merged
merged 2 commits into from
Dec 26, 2017
Merged

STORM-2863: Reset NormalizedResources for tests #2477

merged 2 commits into from
Dec 26, 2017

Conversation

revans2
Copy link
Contributor

@revans2 revans2 commented Dec 21, 2017

No description provided.

@srdo
Copy link
Contributor

srdo commented Dec 21, 2017

@revans2 Could you take a look at #2463? I think it will resolve this issue too.

@srdo srdo mentioned this pull request Dec 21, 2017
Merged
@revans2
Copy link
Contributor Author

revans2 commented Dec 21, 2017

@srdo yes #2463 would resolve the issue too, but it loses most of the reasons why I put in NormalizedResources in the first place. Lets move the discussion over there for now.

@srdo srdo mentioned this pull request Dec 21, 2017
Closed
srdo
srdo reviewed Dec 21, 2017
View reviewed changes
.../apache/storm/scheduler/resource/strategies/scheduling/TestDefaultResourceAwareStrategy.java Outdated
/**
* test if the scheduling logic for the DefaultResourceAwareStrategy is correct
*/
@Test
public void testDefaultResourceAwareStrategySharedMemory() {
NormalizedResources.resetResourceNames();
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to put this in @Before or @After (or ideally a TestRule)? I think it's likely we'll forget to copy this to new tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I will update it

@srdo
Copy link
Contributor

srdo commented Dec 21, 2017

Does https://github.com/revans2/incubator-storm/blob/6aaf8ee7cb042960831547a3ef07ce68a1105970/storm-server/src/test/java/org/apache/storm/scheduler/resource/strategies/scheduling/TestGenericResourceAwareStrategy.java also affect the static fields?

@revans2
Copy link
Contributor Author

revans2 commented Dec 21, 2017

@srdo I moved it over to being a Rule annotation

@srdo
Copy link
Contributor

srdo commented Dec 21, 2017

Thanks, +1 assuming TestGenericResourceAwareStrategy doesn't touch the static fields.

HeartSaVioR
HeartSaVioR approved these changes Dec 26, 2017
View reviewed changes
Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

+1

@asfgit asfgit merged commit 6aaf8ee into apache:master Dec 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srdo srdo srdo left review comments

@HeartSaVioR HeartSaVioR HeartSaVioR approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@revans2 @srdo @HeartSaVioR @asfgit