You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Ethanlm <gi...@git.apache.org> on 2018/03/06 21:55:02 UTC
[GitHub] storm pull request #2587: [STORM-2987] PaceMakerStateStorage should deal wit...
GitHub user Ethanlm opened a pull request:
https://github.com/apache/storm/pull/2587
[STORM-2987] PaceMakerStateStorage should deal with InterruptedException correctly
Explained in https://issues.apache.org/jira/browse/STORM-2987
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Ethanlm/storm STORM-2987
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2587.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2587
----
commit be3f3fb3bf5251681d4d32e201605a6edc38baa8
Author: Ethan Li <et...@...>
Date: 2018-03-06T21:47:29Z
[STORM-2987] PaceMakerStateStorage should deal with InterruptedException correctly
----
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2587
Thanks for the review. Squashed the commits
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2587
Thanks @Ethanlm, merged to master.
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2587
Okay, thanks. I'm a little uncomfortable swallowing exceptions if we don't know why. Ping @revans2 and @HeartSaVioR. Do either of you guys recall why catching and swallowing inside the PacemakerClient.send loop makes more sense than using the rotateClient method?
---
[GitHub] storm pull request #2587: [STORM-2987] PaceMakerStateStorage should deal wit...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2587#discussion_r175557578
--- Diff: storm-client/src/jvm/org/apache/storm/cluster/PaceMakerStateStorage.java ---
@@ -123,12 +124,15 @@ public void set_worker_hb(String path, byte[] data, List<ACL> acls) {
}
LOG.debug("Successful set_worker_hb");
break;
- } catch (Exception e) {
+ } catch (HBExecutionException e) {
if (retry <= 0) {
- throw Utils.wrapInRuntime(e);
+ throw new RuntimeException(e);
}
retry--;
LOG.error("{} Failed to set_worker_hb. Will make {} more attempts.", e.getMessage(), retry);
+ } catch (InterruptedException e) {
+ LOG.debug("set_worker_hb got interrupted: {}", e);
+ throw new RuntimeException(e);
--- End diff --
We need to throw exceptions so that StormTimer can catch it and set the task to inactive.
https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/StormTimer.java#L103-L107
---
[GitHub] storm pull request #2587: [STORM-2987] PaceMakerStateStorage should deal wit...
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2587#discussion_r174388862
--- Diff: storm-client/src/jvm/org/apache/storm/cluster/PaceMakerStateStorage.java ---
@@ -123,12 +124,15 @@ public void set_worker_hb(String path, byte[] data, List<ACL> acls) {
}
LOG.debug("Successful set_worker_hb");
break;
- } catch (Exception e) {
+ } catch (HBExecutionException e) {
if (retry <= 0) {
- throw Utils.wrapInRuntime(e);
+ throw new RuntimeException(e);
}
retry--;
LOG.error("{} Failed to set_worker_hb. Will make {} more attempts.", e.getMessage(), retry);
+ } catch (InterruptedException e) {
+ LOG.debug("set_worker_hb got interrupted: {}", e);
+ throw new RuntimeException(e);
--- End diff --
There is a decision: `retry <= 0`, so throwing is ok to me.
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2587
Thanks for the link. I was new to storm project at that time and I was just pushing our existing internal code to community. Didn't understand very much about it at that time.
---
[GitHub] storm pull request #2587: [STORM-2987] PaceMakerStateStorage should deal wit...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2587
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2587
@Ethanlm Sounds good, thanks.
---
[GitHub] storm pull request #2587: [STORM-2987] PaceMakerStateStorage should deal wit...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2587#discussion_r175556789
--- Diff: storm-client/src/jvm/org/apache/storm/pacemaker/PacemakerClientPool.java ---
@@ -54,25 +54,17 @@ public PacemakerClientPool(Map<String, Object> config) {
}
}
- public HBMessage send(HBMessage m) throws PacemakerConnectionException {
- try {
+ public HBMessage send(HBMessage m) throws InterruptedException {
return getWriteClient().send(m);
- } catch (Exception e) {
- rotateClients();
--- End diff --
Sorry for the late reply as I was on vacation last week. The original code of `getWriteClient().send(m);` actually eats all the exceptions so that no exception will be thrown. I believe this `catch` never did anything.
With this PR, only `InterruptedException` will be thrown from `getWriteClient().send(m)` and I don't think we need to `rotateClients()` when `InterruptedException` happens
---
[GitHub] storm pull request #2587: [STORM-2987] PaceMakerStateStorage should deal wit...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2587#discussion_r175579245
--- Diff: storm-client/src/jvm/org/apache/storm/pacemaker/PacemakerClientPool.java ---
@@ -54,25 +54,17 @@ public PacemakerClientPool(Map<String, Object> config) {
}
}
- public HBMessage send(HBMessage m) throws PacemakerConnectionException {
- try {
+ public HBMessage send(HBMessage m) throws InterruptedException {
return getWriteClient().send(m);
- } catch (Exception e) {
- rotateClients();
--- End diff --
Yea I think we can delete it. Thanks
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2587
@srdo @HeartSaVioR
@revans2 is on vacation. I will ask him after he is back.
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2587
+1
---
[GitHub] storm pull request #2587: [STORM-2987] PaceMakerStateStorage should deal wit...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2587#discussion_r175576651
--- Diff: storm-client/src/jvm/org/apache/storm/pacemaker/PacemakerClientPool.java ---
@@ -54,25 +54,17 @@ public PacemakerClientPool(Map<String, Object> config) {
}
}
- public HBMessage send(HBMessage m) throws PacemakerConnectionException {
- try {
+ public HBMessage send(HBMessage m) throws InterruptedException {
return getWriteClient().send(m);
- } catch (Exception e) {
- rotateClients();
--- End diff --
Thanks, that makes sense. rotateClients looks unused now, does it make sense to keep?
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2587
@srdo
I guess @revans2 and @knusbaum could explain the reason, since `internal` means Oath's own.
---
[GitHub] storm pull request #2587: [STORM-2987] PaceMakerStateStorage should deal wit...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2587#discussion_r175575089
--- Diff: storm-client/src/jvm/org/apache/storm/cluster/PaceMakerStateStorage.java ---
@@ -123,12 +124,15 @@ public void set_worker_hb(String path, byte[] data, List<ACL> acls) {
}
LOG.debug("Successful set_worker_hb");
break;
- } catch (Exception e) {
+ } catch (HBExecutionException e) {
if (retry <= 0) {
- throw Utils.wrapInRuntime(e);
+ throw new RuntimeException(e);
}
retry--;
LOG.error("{} Failed to set_worker_hb. Will make {} more attempts.", e.getMessage(), retry);
+ } catch (InterruptedException e) {
+ LOG.debug("set_worker_hb got interrupted: {}", e);
+ throw new RuntimeException(e);
--- End diff --
Makes sense, thanks.
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2587
@srdo Thanks for pointing it out. Eating all the exceptions might not be a good way and I want to revisit the code and refactor it. Filed a jira https://issues.apache.org/jira/browse/STORM-3017
---
[GitHub] storm issue #2587: [STORM-2987] PaceMakerStateStorage should deal with Inter...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2587
Thanks, I'll go ahead and merge this.
I was wondering if you remember why the try-catch in PacemakerClient.send was added, which made rotateClient redundant? It's from this PR https://github.com/apache/storm/pull/2237, but I'm not sure I follow why it's better than rotateClient?
---