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.

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

STORM-3590: Adds test to validate that GRAS's node sort is stable and… #3217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

govind-menon
Copy link
Contributor

… prevents starvation and fragmentation

@govind-menon govind-menon force-pushed the STORM-3590 branch 4 times, most recently from e46f60b to 97d90bf Compare February 26, 2020 22:47
// schedule gpu topology
scheduler.schedule(topologies, cluster);

cluster.addTopologyToClusterToBeScheduled(createTestStormTopology(stormTopologyNoGpu, 10, noGpu, nonGpuconf));
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like a good idea to add a new function in the regular code solely for unit test. We don't need to add this function to achieve what we want. We can follow this https://github.com/apache/storm/blob/master/storm-server/src/test/java/org/apache/storm/scheduler/resource/TestResourceAwareScheduler.java#L504-L511

@@ -76,6 +76,14 @@ public Topologies(Topologies src) {
return ret;
}

public void addTopology(TopologyDetails details) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is supposed to be immutable class. just create another instance of topologies using constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't add methods to regular code while it's only used in unit test

// schedule gpu topology
scheduler.schedule(topologies, cluster);

topologies.addTopology(createTestStormTopology(stormTopologyNoGpu, 10, noGpu, nonGpuconf));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is supposed to be immutable class. just create another instance of topologies using constructor.

try Topologies topologies = new Topologies(topo[0], topo[1]);

@@ -76,6 +76,14 @@ public Topologies(Topologies src) {
return ret;
}

public void addTopology(TopologyDetails details) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't add methods to regular code while it's only used in unit test


// should schedule gpu and noGpu successfully
scheduler = new ResourceAwareScheduler();
nonGpuconf.put(DaemonConfig.RESOURCE_AWARE_SCHEDULER_MAX_TOPOLOGY_SCHEDULING_ATTEMPTS, 1); // allows no evictions
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not able to indicate if eviction happens or not.
Waiting on #3213 so we can have a right way to detect eviction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants