You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by redsanket <gi...@git.apache.org> on 2016/03/08 20:09:50 UTC
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
GitHub user redsanket opened a pull request:
https://github.com/apache/storm/pull/1194
[STORM-1609] Netty Client is not best effort delivery on failed Connection
If Worker-A has connection to Worker-B that is unused ( and if Worker-B restarted), we drop messages because Channel is not in good state. Can we avoid message drop until we succeed in making new connection or a timeout?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/redsanket/storm netty-best-effort
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1194.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 #1194
----
commit 54b6ac4975582bb6b3ef3511369e52ad81db05b3
Author: Sanket <sc...@untilservice-lm>
Date: 2016-03-08T19:06:39Z
netty loss of messages resolution
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55440955
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -151,10 +164,35 @@
bootstrap = createClientBootstrap(factory, bufferSize, stormConf);
dstAddress = new InetSocketAddress(host, port);
dstAddressPrefixedName = prefixedName(dstAddress);
+ launchChannelAliveThread();
scheduleConnect(NO_DELAY_MS);
batcher = new MessageBuffer(messageBatchSize);
}
+ /**
+ * This thread helps us to check for channel connection periodically.
+ * This is performed just to know whether the destination address
+ * is alive or attempts to refresh connections if not alive. This
+ * solution is better than what we have now in case of a bad channel.
+ */
+ private void launchChannelAliveThread() {
+ // netty TimerTask is already defined and hence a fully
+ // qualified name
+ timer.schedule(new java.util.TimerTask() {
+ public void run() {
+ try {
+ LOG.debug("running timer task, address {}", dstAddress);
+ if(closing) {
+ this.cancel();
--- End diff --
fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1194#issuecomment-193948629
+1 looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55432309
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -108,6 +110,13 @@
private final AtomicInteger messagesLost = new AtomicInteger(0);
/**
+ * Periodically checks for connected channel in order to avoid loss
+ * of messages
+ */
+ private final long CHANNEL_ALIVE_INTERVAL_MS = 30000L;
+
--- End diff --
minor: extra line
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on the pull request:
https://github.com/apache/storm/pull/1194#issuecomment-194848650
Agree, that is a bigger change and I will have a follow up jira for it. Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on the pull request:
https://github.com/apache/storm/pull/1194#issuecomment-194000825
@d2r Addressed your comments
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55432071
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -151,10 +164,35 @@
bootstrap = createClientBootstrap(factory, bufferSize, stormConf);
dstAddress = new InetSocketAddress(host, port);
dstAddressPrefixedName = prefixedName(dstAddress);
+ launchChannelAliveThread();
scheduleConnect(NO_DELAY_MS);
batcher = new MessageBuffer(messageBatchSize);
}
+ /**
+ * This thread helps us to check for channel connection periodically.
+ * This is performed just to know whether the destination address
+ * is alive or attempts to refresh connections if not alive. This
+ * solution is better than what we have now in case of a bad channel.
+ */
+ private void launchChannelAliveThread() {
+ // netty TimerTask is already defined and hence a fully
+ // qualified name
+ timer.schedule(new java.util.TimerTask() {
+ public void run() {
+ try {
+ LOG.debug("running timer task, address {}", dstAddress);
+ if(closing) {
+ this.cancel();
--- End diff --
Just to be cleaner and avoid a possible race, let's return here so we don't call `getConnectedChannel`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1194#issuecomment-193972568
+1. We should also apply this to 1.x-branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1194#issuecomment-195849096
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55561657
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -151,10 +159,43 @@
bootstrap = createClientBootstrap(factory, bufferSize, stormConf);
dstAddress = new InetSocketAddress(host, port);
dstAddressPrefixedName = prefixedName(dstAddress);
+ launchChannelAliveThread();
scheduleConnect(NO_DELAY_MS);
batcher = new MessageBuffer(messageBatchSize);
}
+ /**
+ * This thread helps us to check for channel connection periodically.
+ * This is performed just to know whether the destination address
+ * is alive or attempts to refresh connections if not alive. This
+ * solution is better than what we have now in case of a bad channel.
+ */
+ private void launchChannelAliveThread() {
+ // netty TimerTask is already defined and hence a fully
+ // qualified name
+ if (timer == null) {
--- End diff --
@d2r @revan2 reverted back to static initialization
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55431809
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -151,10 +164,35 @@
bootstrap = createClientBootstrap(factory, bufferSize, stormConf);
dstAddress = new InetSocketAddress(host, port);
dstAddressPrefixedName = prefixedName(dstAddress);
+ launchChannelAliveThread();
scheduleConnect(NO_DELAY_MS);
batcher = new MessageBuffer(messageBatchSize);
}
+ /**
+ * This thread helps us to check for channel connection periodically.
+ * This is performed just to know whether the destination address
+ * is alive or attempts to refresh connections if not alive. This
+ * solution is better than what we have now in case of a bad channel.
+ */
+ private void launchChannelAliveThread() {
--- End diff --
Let's move the code in the static block above down here at the beginning of `launchChannelAliveThread`, so that as @revans2 suggests, the stack trace will not be so confusing.
```Java
if (timer == null) {
synchronized (Client.class) {
if (timer == null) {
timer = new Timer("Netty-ChannelAlive-Timer", true);
}
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55441002
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -151,10 +164,35 @@
bootstrap = createClientBootstrap(factory, bufferSize, stormConf);
dstAddress = new InetSocketAddress(host, port);
dstAddressPrefixedName = prefixedName(dstAddress);
+ launchChannelAliveThread();
scheduleConnect(NO_DELAY_MS);
batcher = new MessageBuffer(messageBatchSize);
}
+ /**
+ * This thread helps us to check for channel connection periodically.
+ * This is performed just to know whether the destination address
+ * is alive or attempts to refresh connections if not alive. This
+ * solution is better than what we have now in case of a bad channel.
+ */
+ private void launchChannelAliveThread() {
--- End diff --
fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/1194#issuecomment-194008759
Looks good +1.
I see unrelated failures in supervisor-test and transactional-test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/1194#issuecomment-193979685
Had some usability concerns, but otherwise looks good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55439564
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -151,10 +164,35 @@
bootstrap = createClientBootstrap(factory, bufferSize, stormConf);
dstAddress = new InetSocketAddress(host, port);
dstAddressPrefixedName = prefixedName(dstAddress);
+ launchChannelAliveThread();
scheduleConnect(NO_DELAY_MS);
batcher = new MessageBuffer(messageBatchSize);
}
+ /**
+ * This thread helps us to check for channel connection periodically.
+ * This is performed just to know whether the destination address
+ * is alive or attempts to refresh connections if not alive. This
+ * solution is better than what we have now in case of a bad channel.
+ */
+ private void launchChannelAliveThread() {
--- End diff --
Good suggestion, I will do that thank you d2r
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1194
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the pull request:
https://github.com/apache/storm/pull/1194#issuecomment-194859547
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by hustfxj <gi...@git.apache.org>.
Github user hustfxj commented on the pull request:
https://github.com/apache/storm/pull/1194#issuecomment-194629551
+1. I have a suggestion. we drop messages because Channel is not in good state. I hope we should put the messages into buffer when Channel is not in good state. Of course, we should drop the buffer if the channel is still not in good state.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55546601
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -151,10 +159,43 @@
bootstrap = createClientBootstrap(factory, bufferSize, stormConf);
dstAddress = new InetSocketAddress(host, port);
dstAddressPrefixedName = prefixedName(dstAddress);
+ launchChannelAliveThread();
scheduleConnect(NO_DELAY_MS);
batcher = new MessageBuffer(messageBatchSize);
}
+ /**
+ * This thread helps us to check for channel connection periodically.
+ * This is performed just to know whether the destination address
+ * is alive or attempts to refresh connections if not alive. This
+ * solution is better than what we have now in case of a bad channel.
+ */
+ private void launchChannelAliveThread() {
+ // netty TimerTask is already defined and hence a fully
+ // qualified name
+ if (timer == null) {
--- End diff --
@d2r, @revans2 looking at it it seems a valid explanation. Would like to know whether it is better to revert to earlier static initialization
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by bugzmanov <gi...@git.apache.org>.
Github user bugzmanov commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55464579
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -151,10 +159,43 @@
bootstrap = createClientBootstrap(factory, bufferSize, stormConf);
dstAddress = new InetSocketAddress(host, port);
dstAddressPrefixedName = prefixedName(dstAddress);
+ launchChannelAliveThread();
scheduleConnect(NO_DELAY_MS);
batcher = new MessageBuffer(messageBatchSize);
}
+ /**
+ * This thread helps us to check for channel connection periodically.
+ * This is performed just to know whether the destination address
+ * is alive or attempts to refresh connections if not alive. This
+ * solution is better than what we have now in case of a bad channel.
+ */
+ private void launchChannelAliveThread() {
+ // netty TimerTask is already defined and hence a fully
+ // qualified name
+ if (timer == null) {
--- End diff --
This looks a lot like racy(incorrect) version of:
https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java
So you might end up with two instances of timer running 1 job each
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1609] Netty Client is not best effort d...
Posted by redsanket <gi...@git.apache.org>.
Github user redsanket commented on a diff in the pull request:
https://github.com/apache/storm/pull/1194#discussion_r55441072
--- Diff: storm-core/src/jvm/org/apache/storm/messaging/netty/Client.java ---
@@ -108,6 +110,13 @@
private final AtomicInteger messagesLost = new AtomicInteger(0);
/**
+ * Periodically checks for connected channel in order to avoid loss
+ * of messages
+ */
+ private final long CHANNEL_ALIVE_INTERVAL_MS = 30000L;
+
--- End diff --
fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---