Some changes to address blocking after a node is lost #2

daschl · 2014-04-23T06:19:48Z

we've cached them just 2 lines above, do you think we need to access them fresh all the time - why?

daschl · 2014-04-23T06:19:52Z

we've cached them just 2 lines above, do you think we need to access them fresh all the time - why?

daschl · 2014-04-23T06:20:22Z

what would be the benefit of this approach to the old one?

selectedKeys is touched by multiple threads, a foreach loop in java is not thread safe as it modifies the set while it loops, i believe instead of using the iterator methods it's using the poll() underneath the covers and can cause "arbitrary, nondeterministic" behavior and/or ConcurrentModificationExceptions. The iterator is the appropriate way to loop through a selectedKeys set in this scenario. I believe that this is an important change based on best practices, and that it only presents problems under high load and thus is very difficult to chase down.
Furthermore removing the key immediately using iterator.remove instead of doing a set.clear() because who knows what has been added to the keys since you first made a local set

I see. this code was in place long time ago, It looks good from a code-review perspective, but I'll do some benchmarking to see if it has a perf impact.

sveesible · 2014-04-23T02:56:56Z

This was adding the op right back onto the node that it just attempted to redistribute away from, and then letting the op go down into the next set of logic to be cloned and redistributed doubly, instead of adding the operation with it's key to get a new node and then returning.
introduced here: e6892a0

That was actually the intention, that we get it to the node directly.

I think the bug here is the missing return statement, the addOperation() call is correct. It should go to the same node, because the logic at this point already took it out of the retry queue where it has been put before (for example if the auth latch for the node doesn't complete).

but in the case of a dead node that hasn't been detected yet as dead, wouldn't those writes want to get redistributed, is there a check you can do on the node's current interestedops is OP_CONNECT node.getSk().interestOps() == SelectionKey.OP_CONNECT

@sveesible I guess we can remove the
if (op.getState() == OperationState.WRITE_QUEUED) {
addOperation(op.getHandlingNode(), op);
}
altogether then? It was just meant as a optimization, if it gets removed the regular cloning will happen as before. what do you think?

removing it would also help because down the code we check for multigets, which would not be covered by your change in the line (just one .next()) call

doesn't write mean set, and are there multi-sets? the write op is an instance of SingleKeyedOperation, so the .next() would get the only key

At the very least the return statement right?

no, WRITE_QUEUED means its has been in the write queue for that node before it got redistributed, not tied to any specific operation type.

it could be a single or multi key operation (like the multiget below).
I think let's add the return; for now in there alone and then we'll see how it works out. If you are still running into troubles when we have it merged in we can remove the optimization later - not sure this will be an issue though.

sveesible · 2014-04-22T03:05:59Z

this is petty, keeping consistent with queueReconnect closing the socket

I see.. looking at it in more detail, it could be that the other code is overly verbose, since the ch is a SocketChannel, and I guess if the channel gets closed the underlying socket will be too?

I think that in certain cases with selectors involved the ch.close() will wait until the next select operation on the channel, but I'm not sure if when it's in a bad state any more selects will be allowed to happen

Not sure this will provide any real benefit. Let's get the other things in order first and if we see a specific reason to change this lets do it afterwards.

I checked the code and we have both styles in certain places.

sveesible · 2014-04-22T02:45:33Z

this seems like the big one in my testing that clogs up the mechanism when a server node is lost suddenly

Please see this change set where the bug was introduced last July
00f2e78

yes, that looks like a typo/bug.

http://review.couchbase.org/#/c/36221/

-Original file line number
+Diff line change
@@ Expand Up / @@ -407,17 +407,19 @@ public void handleIO() throws IOException { @@
         int selected = selector.select(delay);
         Set<SelectionKey> selectedKeys = selector.selectedKeys();
-        if (selectedKeys.isEmpty() && !shutDown) {
+        if (selector.selectedKeys().isEmpty() && !shutDown) {
           handleEmptySelects();
         } else {
           getLogger().debug("Selected %d, selected %d keys", selected,
-            selectedKeys.size());
+            selector.selectedKeys().size());
           emptySelects = 0;
-          for (SelectionKey sk : selectedKeys) {
+          Iterator<SelectionKey> iterator = selector.selectedKeys().iterator();
+          while (iterator.hasNext())  {
+            SelectionKey sk = iterator.next();
             handleIO(sk);
+            iterator.remove();
           }
-          selectedKeys.clear();
         }
         handleOperationalTasks();
@@ Expand Down Expand Up / @@ -1005,10 +1007,11 @@ public void redistributeOperation(Operation op) { @@
           return;
         }
-        // The operation gets redistributed but has never been actually written,
-        // it we just straight re-add it without cloning.
+        // The operation gets redistributed but has never actually been written,
+        // then we just straight re-add it without cloning.
         if (op.getState() == OperationState.WRITE_QUEUED) {
-          addOperation(op.getHandlingNode(), op);
+          addOperation(((KeyedOperation)op).getKeys().iterator().next(), op);
+          return;
         }
         if (op instanceof MultiGetOperationImpl) {
@@ Expand Down Expand Up @@
         final MemcachedNode node) {
         if (ch != null && !ch.isConnected() && !ch.isConnectionPending()) {
           try {
-            ch.close();
+            ch.socket().close();
           } catch (IOException e) {
             getLogger().error("Exception closing channel: %s", node, e);
           }
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
       private final long opQueueMaxBlockTime;
       private final long authWaitTime;
       private final ConnectionFactory connectionFactory;
-      private AtomicInteger reconnectAttempt = new AtomicInteger(1);
+      private AtomicInteger reconnectAttempt = new AtomicInteger(0);
       private SocketChannel channel;
       private int toWrite = 0;
       protected Operation optimizedOp = null;
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
           if(getCb != null) {
             rv.add(get(k, getCb));
           } else if(getsCb != null) {
-            rv.add(get(k, getCb));
+            rv.add(gets(k, getsCb));
           } else {
             rv.add(replicaGet(k, ((ReplicaGetOperationImpl)op).getReplicaIndex() ,replicaGetCb));
           }
@@ Expand Down @@

Some changes to address blocking after a node is lost #2

Are you sure you want to change the base?

Uh oh!

Some changes to address blocking after a node is lost #2

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!