You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by GitBox <gi...@apache.org> on 2020/10/29 17:55:32 UTC
[GitHub] [storm] bipinprasad opened a new pull request #3342: [STORM-3708] Add TopologyID to log messages in ConstraintSolverConfig
bipinprasad opened a new pull request #3342:
URL: https://github.com/apache/storm/pull/3342
## What is the purpose of the change
*Log messages in ConstraintSolverConfig do not show the TopologyId. This makes it harder to track problems when there are several topologies running on the cluster.*
## How was the change tested
*Run unit tests in storm-server package*
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3342: [STORM-3708] Add TopologyID to log messages in ConstraintSolverConfig
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3342:
URL: https://github.com/apache/storm/pull/3342#discussion_r524360893
##########
File path: storm-server/src/test/java/org/apache/storm/scheduler/resource/strategies/scheduling/TestConstraintSolverStrategy.java
##########
@@ -551,7 +550,7 @@ public void testIntegrationWithRAS() {
public void testZeroExecutorScheduling() {
ConstraintSolverStrategy cs = new ConstraintSolverStrategy();
cs.prepare(new HashMap<>());
- Map<String, Object> topoConf = new HashMap<>();
+ Map<String, Object> topoConf = Utils.readDefaultConfig();
Review comment:
I noticed this was changed in the acker distribution PR https://github.com/apache/storm/pull/3346. I would remove this from this PR since it is irrelevant. It will be easier to understand the change when we look back the history in the future. Thanks
##########
File path: storm-server/src/test/java/org/apache/storm/scheduler/resource/strategies/scheduling/TestConstraintSolverStrategy.java
##########
@@ -551,7 +550,7 @@ public void testIntegrationWithRAS() {
public void testZeroExecutorScheduling() {
ConstraintSolverStrategy cs = new ConstraintSolverStrategy();
cs.prepare(new HashMap<>());
- Map<String, Object> topoConf = new HashMap<>();
+ Map<String, Object> topoConf = Utils.readDefaultConfig();
Review comment:
Why are we changing this in this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3342: [STORM-3708] Add TopologyID to log messages in ConstraintSolverConfig
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3342:
URL: https://github.com/apache/storm/pull/3342#discussion_r524228493
##########
File path: storm-server/src/test/java/org/apache/storm/scheduler/resource/strategies/scheduling/TestConstraintSolverStrategy.java
##########
@@ -551,7 +550,7 @@ public void testIntegrationWithRAS() {
public void testZeroExecutorScheduling() {
ConstraintSolverStrategy cs = new ConstraintSolverStrategy();
cs.prepare(new HashMap<>());
- Map<String, Object> topoConf = new HashMap<>();
+ Map<String, Object> topoConf = Utils.readDefaultConfig();
Review comment:
This test for ConstraintSolverStrategy is different internally.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3342: [STORM-3708] Add TopologyID to log messages in ConstraintSolverConfig
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3342:
URL: https://github.com/apache/storm/pull/3342#discussion_r530798942
##########
File path: storm-server/src/test/java/org/apache/storm/scheduler/resource/strategies/scheduling/TestConstraintSolverStrategy.java
##########
@@ -551,7 +550,7 @@ public void testIntegrationWithRAS() {
public void testZeroExecutorScheduling() {
ConstraintSolverStrategy cs = new ConstraintSolverStrategy();
cs.prepare(new HashMap<>());
- Map<String, Object> topoConf = new HashMap<>();
+ Map<String, Object> topoConf = Utils.readDefaultConfig();
Review comment:
Reverted
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3342: [STORM-3708] Add TopologyID to log messages in ConstraintSolverConfig
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3342:
URL: https://github.com/apache/storm/pull/3342#discussion_r523903303
##########
File path: storm-server/src/test/java/org/apache/storm/scheduler/resource/strategies/scheduling/TestConstraintSolverStrategy.java
##########
@@ -551,7 +550,7 @@ public void testIntegrationWithRAS() {
public void testZeroExecutorScheduling() {
ConstraintSolverStrategy cs = new ConstraintSolverStrategy();
cs.prepare(new HashMap<>());
- Map<String, Object> topoConf = new HashMap<>();
+ Map<String, Object> topoConf = Utils.readDefaultConfig();
Review comment:
Why are we change this in this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm merged pull request #3342: [STORM-3708] Add TopologyID to log messages in ConstraintSolverConfig
Posted by GitBox <gi...@apache.org>.
Ethanlm merged pull request #3342:
URL: https://github.com/apache/storm/pull/3342
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org