STORM-1546: Adding Read and Write Aggregations for Pacemaker to make it HA compatible #1710

knusbaum · 2016-09-23T07:35:56Z

No description provided.

d2r

Mostly minor clean-up comments.

We do need to update the Pacemaker documentation since we are replacing the old host config that is documented there.

d2r · 2016-09-23T19:47:18Z

conf/defaults.yaml

@@ -276,6 +276,7 @@ resource.aware.scheduler.priority.strategy: "org.apache.storm.scheduler.resource
 dev.zookeeper.path: "/tmp/dev-storm-zookeeper"

 pacemaker.host: "localhost"
+pacemaker.servers: []


Let's remove pacemaker.host if it is no longer used.

d2r · 2016-09-23T20:16:09Z

storm-core/src/jvm/org/apache/storm/pacemaker/PacemakerClientPool.java

+
+public class PacemakerClientPool {
+
+        private static final Logger LOG = LoggerFactory.getLogger(PacemakerClientPool.class);


check indentation

d2r · 2016-09-23T20:20:38Z

storm-core/src/jvm/org/apache/storm/pacemaker/PacemakerClientPool.java

+        }
+        return client;
+    }
+


minor: extra line

d2r · 2016-09-23T20:33:26Z

storm-core/src/jvm/org/apache/storm/cluster/PaceMakerStateStorage.java

@@ -160,13 +179,23 @@ public void set_worker_hb(String path, byte[] data, List<ACL> acls) {
        int retry = maxRetries;
        while (true) {
            try {
+                HashSet<String> retSet = new HashSet<>();
+                int latest_time_secs = 0;


latest_time_sec not used?

d2r · 2016-09-23T20:34:28Z

storm-core/src/jvm/org/apache/storm/cluster/PaceMakerStateStorageFactory.java

@@ -18,48 +18,22 @@
 package org.apache.storm.cluster;

 import org.apache.storm.pacemaker.PacemakerClient;
+import org.apache.storm.pacemaker.PacemakerClientPool;


Remove PacemakerClient import, as it's not used?

d2r · 2016-09-23T20:42:07Z

storm-core/src/jvm/org/apache/storm/pacemaker/PacemakerClientHandler.java

@@ -69,7 +69,7 @@ else if(evm instanceof HBMessage) {

    @Override
    public void exceptionCaught(ChannelHandlerContext ctx, ExceptionEvent event) {
-        LOG.error("Connection to pacemaker failed", event.getCause());
+        LOG.error("Connection to pacemaker failed. Trying to reconnect {}", event.getCause().getMessage());


We don't want to log the stack here, only the message?

I don't think we want the stack. It ends up absolutely filling Nimbus logs with relatively useless messages when a Pacemaker node goes down. I'm open to ideas, though, because a trace for an NPE or something would be nice.

If we know which exceptions we want to ignore, perhaps just IOExceptions, then we can explicitly blacklist them, and even log them under warn, instead of error.

d2r · 2016-09-23T20:48:11Z

storm-core/test/jvm/org/apache/storm/PaceMakerStateStorageFactoryTest.java


-    private PacemakerClient clientProxy;
+public class PaceMakerStateStorageFactoryTest {
+    private static Logger LOG = LoggerFactory.getLogger(PaceMakerStateStorageFactoryTest.class);


We don't seem to be logging anything in this test. Do we need the Logger?

knusbaum · 2016-09-26T21:33:41Z

Addressed your comments. Not sure why they have not disappeared.

revans2

Overall looks good, just a nit, and possibly a backwards compatibility issue.

Also I think we need to update the docs to explain HA and how it works/how to configure it.

revans2 · 2016-09-27T15:16:24Z

storm-core/src/jvm/org/apache/storm/Config.java

-    @isString
-    public static final String PACEMAKER_HOST = "pacemaker.host";
+    @isStringList
+    public static final String PACEMAKER_SERVERS = "pacemaker.servers";


This is a breaking change that would be good to go back to 1.x as well. I am OK with this, because I don't think anyone is really using pacemaker, but I would like to hear others opinions on it.

I'm in favor of backward compatibility change for this, like nimbus.host and nimbus.seeds. For 2.0 we don't need to keep nimbus.host and pacemaker.host but 1.x still need to have it.

@HeartSaVioR Yep, the 1.x-branch PR will leave pacemaker.host in, but have it deprecated.

revans2 · 2016-09-27T15:20:48Z

storm-core/src/jvm/org/apache/storm/pacemaker/PacemakerClientHandler.java

@@ -69,7 +69,7 @@ else if(evm instanceof HBMessage) {

    @Override
    public void exceptionCaught(ChannelHandlerContext ctx, ExceptionEvent event) {
-        LOG.error("Connection to pacemaker failed", event.getCause());
+        LOG.error("Connection to pacemaker failed. Trying to reconnect {}", event.getCause().getMessage());


If we know which exceptions we want to ignore, perhaps just IOExceptions, then we can explicitly blacklist them, and even log them under warn, instead of error.

revans2 · 2016-09-27T15:21:47Z

storm-core/src/jvm/org/apache/storm/pacemaker/PacemakerClientPool.java

+        this.config = config;
+        List<String> serverList = (List<String>)config.get(Config.PACEMAKER_SERVERS);
+        if(serverList == null) {
+            serverList = new ArrayList<String>();


nit: I don't think <String> is needed just <>

and in the following few lines too.

revans2 · 2016-09-27T15:22:41Z

storm-core/src/jvm/org/apache/storm/pacemaker/PacemakerClientPool.java

+
+    public List<HBMessage> sendAll(HBMessage m) throws PacemakerConnectionException {
+        List<HBMessage> responses = new ArrayList<HBMessage>();
+        LOG.info("Using servers: {}", servers);


Perhaps this should be debug (especially if we are worried about filling the logs)

revans2 · 2016-09-27T20:48:02Z

storm-core/src/jvm/org/apache/storm/cluster/PaceMakerStateStorage.java

-                if(ret == null) {
-                    throw new HBExecutionException("Failed to get a response.");
-                }
-                LOG.debug("Successful get_worker_hb");
                return ret;


Do we need the ret == null check before this? Is it possible to get more then one response and all of them are bogus?

Yes, I wasn't quite sure how we wanted to handle this, NULL responses are valid, and right now the code treats mismatched responses as if they were NULL. We can distinguish between them if we want the behavior to differ whether the pacemakers returned NULL details or returned an invalid message. Throwing on null is not what we wanted, though, since it will cause Nimbus to crash when nimbus tries to read a topology's heartbeats before workers have started sending them.

Maybe we want to add some logic so that if ALL pacemakers return an invalid response, then we throw?

I think that is best, if all of them return something invalid or we get nothing back at all then we throw.

knusbaum · 2016-09-30T18:11:58Z

@d2r @revans2 Any additional comments?

revans2 · 2016-10-03T14:54:17Z

+1

knusbaum · 2016-10-04T18:55:19Z

Documentation updated.

d2r · 2016-10-05T21:29:33Z

Thanks for doc update. +1

knusbaum added 10 commits September 6, 2016 16:04

Inspecting travis's maven cache

c71314f

Working on Pacemaker HA

efa5468

cleanup.

f46cfc1

Merge branch 'master' of http://git-wip-us.apache.org/repos/asf/storm

0c00292

Fix ups.

6cd9661

Fix ups.

6a8be9f

Cleaning up tests.

523317b

Cleaning up.

410ef3a

Kick travis

158228b

Fixing travis, adding license to new code.

6a657f1

knusbaum closed this Sep 23, 2016

knusbaum reopened this Sep 23, 2016

d2r requested changes Sep 23, 2016

View reviewed changes

Addressing comments by d2r

837591d

revans2 reviewed Sep 27, 2016

View reviewed changes

knusbaum added 2 commits September 27, 2016 12:38

Addressing more comments.

230e64e

More updates.

7252848

revans2 reviewed Sep 27, 2016

View reviewed changes

Adding throw if we get no valid responses.

3b8472b

Updating Pacemaker.md

4501052

asfgit merged commit 4501052 into apache:master Oct 5, 2016

HeartSaVioR mentioned this pull request Oct 6, 2016

STORM-1546: Adding Read and Write Aggregations for Pacemaker to make it HA compatible (1.x-branch) #1720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STORM-1546: Adding Read and Write Aggregations for Pacemaker to make it HA compatible #1710

STORM-1546: Adding Read and Write Aggregations for Pacemaker to make it HA compatible #1710

knusbaum commented Sep 23, 2016

d2r left a comment

d2r Sep 23, 2016 •

edited

Loading

d2r Sep 23, 2016

d2r Sep 23, 2016

d2r Sep 23, 2016

d2r Sep 23, 2016

d2r Sep 23, 2016

knusbaum Sep 26, 2016

revans2 Sep 27, 2016

d2r Sep 23, 2016

knusbaum commented Sep 26, 2016

revans2 left a comment

revans2 Sep 27, 2016

HeartSaVioR Sep 27, 2016

knusbaum Sep 29, 2016

revans2 Sep 27, 2016

revans2 Sep 27, 2016

revans2 Sep 27, 2016

revans2 Sep 27, 2016

revans2 Sep 27, 2016

knusbaum Sep 27, 2016 •

edited

Loading

revans2 Sep 27, 2016

knusbaum commented Sep 30, 2016

revans2 commented Oct 3, 2016

knusbaum commented Oct 4, 2016

d2r commented Oct 5, 2016


		public class PacemakerClientPool {

		private static final Logger LOG = LoggerFactory.getLogger(PacemakerClientPool.class);

STORM-1546: Adding Read and Write Aggregations for Pacemaker to make it HA compatible #1710

STORM-1546: Adding Read and Write Aggregations for Pacemaker to make it HA compatible #1710

Conversation

knusbaum commented Sep 23, 2016

d2r left a comment

Choose a reason for hiding this comment

d2r Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knusbaum commented Sep 26, 2016

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knusbaum Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knusbaum commented Sep 30, 2016

revans2 commented Oct 3, 2016

knusbaum commented Oct 4, 2016

d2r commented Oct 5, 2016

d2r Sep 23, 2016 •

edited

Loading

knusbaum Sep 27, 2016 •

edited

Loading