-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversation
e46f60b
to
97d90bf
Compare
// schedule gpu topology | ||
scheduler.schedule(topologies, cluster); | ||
|
||
cluster.addTopologyToClusterToBeScheduled(createTestStormTopology(stormTopologyNoGpu, 10, noGpu, nonGpuconf)); |
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.
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
…prevents starvation and fragmentation
97d90bf
to
dbb1d42
Compare
@@ -76,6 +76,14 @@ public Topologies(Topologies src) { | |||
return ret; | |||
} | |||
|
|||
public void addTopology(TopologyDetails details) { |
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.
This is supposed to be immutable class. just create another instance of topologies using constructor.
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.
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)); |
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.
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) { |
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.
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 |
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.
This is not able to indicate if eviction happens or not.
Waiting on #3213 so we can have a right way to detect eviction.
… prevents starvation and fragmentation