KetamaNodeLocator's getSequence() bug

[GoogleCodeExporter]

What version of the product are you using? 2.3.1
On what operating system? windows xp, java6

Tell me more...

I've tested follwing code

package jp.hange.session.benchmark.memcached;

import java.io.IOException;
import java.net.SocketAddress;
import java.nio.ByteBuffer;
import java.nio.channels.SelectionKey;
import java.nio.channels.SocketChannel;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Iterator;
import java.util.List;

import org.junit.Before;
import org.junit.Test;

import net.spy.memcached.HashAlgorithm;
import net.spy.memcached.KetamaNodeLocator;
import net.spy.memcached.MemcachedNode;
import net.spy.memcached.ops.Operation;

public class KetamaNodeLocatorTest {

    private KetamaNodeLocator ketamaNodeLocator;
    List<MemcachedNode> memcachedNodeList;

    @Before
    public void setUp() throws Exception {

        MemcachedNode node1 = new MockMemcachedNode("11211");
        MemcachedNode node2 = new MockMemcachedNode("11212");
        MemcachedNode node3 = new MockMemcachedNode("11213");       

        memcachedNodeList = new ArrayList<MemcachedNode>();
        memcachedNodeList.add(node1);
        memcachedNodeList.add(node2);
        memcachedNodeList.add(node3);

        ketamaNodeLocator = new 
KetamaNodeLocator(memcachedNodeList, HashAlgorithm.KETAMA_HASH);
    }

    /**
     *  result
    12:2
    13:2
    26:2
    29:2
    41:2
    78:2
    85:2
    86:2
    97:2
    **/

    @Test
    public void getNode() throws Exception {
        for(int k = 0 ; k < 100; k++){
            String result = null;
            int equalCount = 0;
            for(Iterator<MemcachedNode> 
i=ketamaNodeLocator.getSequence(String.valueOf(k));i.hasNext(); ) {
                MemcachedNode n=i.next();
                if(n.toString().equals(result)){
                    equalCount++;
                }
                result = n.toString();
            }

            if(equalCount == 2){
                System.out.println(k + ":" + equalCount );
            }
        }
    }

    private String testKey = "12";
    @Test
    public void getNode2() throws Exception {
        for(Iterator<MemcachedNode> 
i=ketamaNodeLocator.getSequence(testKey);i.hasNext(); ) {
            MemcachedNode n=i.next();
            System.out.println(n);
        }
    }
}

class MockMemcachedNode implements MemcachedNode {

    private String socketAddress;

    public MockMemcachedNode(String socketAddress){
        this.socketAddress = socketAddress;
    }

    @Override
    public String toString() {
        return socketAddress;
    }

    public SocketAddress getSocketAddress() {
        // TODO Auto-generated method stub
        return new SocketAddress(){
            @Override
            public String toString() {
                return socketAddress;
            }
        };
    }

    public void addOp(Operation op) {
        // TODO Auto-generated method stub

    }

    public void connected() {
        // TODO Auto-generated method stub

    }

    public void copyInputQueue() {
        // TODO Auto-generated method stub

    }

    public Collection<Operation> destroyInputQueue() {
        // TODO Auto-generated method stub
        return null;
    }

    public void fillWriteBuffer(boolean optimizeGets) {
        // TODO Auto-generated method stub

    }

    public void fixupOps() {
        // TODO Auto-generated method stub

    }

    public int getBytesRemainingToWrite() {
        // TODO Auto-generated method stub
        return 0;
    }

    public SocketChannel getChannel() {
        // TODO Auto-generated method stub
        return null;
    }

    public Operation getCurrentReadOp() {
        // TODO Auto-generated method stub
        return null;
    }

    public Operation getCurrentWriteOp() {
        // TODO Auto-generated method stub
        return null;
    }

    public ByteBuffer getRbuf() {
        // TODO Auto-generated method stub
        return null;
    }

    public int getReconnectCount() {
        // TODO Auto-generated method stub
        return 0;
    }

    public int getSelectionOps() {
        // TODO Auto-generated method stub
        return 0;
    }

    public SelectionKey getSk() {
        // TODO Auto-generated method stub
        return null;
    }

    public ByteBuffer getWbuf() {
        // TODO Auto-generated method stub
        return null;
    }

    public boolean hasReadOp() {
        // TODO Auto-generated method stub
        return false;
    }

    public boolean hasWriteOp() {
        // TODO Auto-generated method stub
        return false;
    }

    public boolean isActive() {
        // TODO Auto-generated method stub
        return false;
    }

    public void reconnecting() {
        // TODO Auto-generated method stub

    }

    public void registerChannel(SocketChannel ch, SelectionKey 
selectionKey) {
        // TODO Auto-generated method stub

    }

    public Operation removeCurrentReadOp() {
        // TODO Auto-generated method stub
        return null;
    }

    public Operation removeCurrentWriteOp() {
        // TODO Auto-generated method stub
        return null;
    }

    public void setChannel(SocketChannel to) {
        // TODO Auto-generated method stub

    }

    public void setSk(SelectionKey to) {
        // TODO Auto-generated method stub

    }

    public void setupResend() {
        // TODO Auto-generated method stub

    }

    public void transitionWriteItem() {
        // TODO Auto-generated method stub

    }

    public int writeSome() throws IOException {
        // TODO Auto-generated method stub
        return 0;
    }

}

I've create method of getNode() to find specific key value.

if the specific key value is needed for testing KetamaNodeLocator's 
getSequence() method.

getNode2() method's result is following

11211
11211
11211
11211
11211

Because getSequence()'s value is all same, though node1 were disconnected, 
another node is unavailable.

sorry my poor english.

Original issue reported on code.google.com by mayd...@gmail.com on 20 May 2009 at 10:38

[GoogleCodeExporter]

At result, if i used KetataConnectionFactory and a memcached server gone down, 
some 
key was permanently to be unavailable.

Original comment by mayd...@gmail.com on 20 May 2009 at 10:44

[GoogleCodeExporter]

I don't think this is testing what you think it's testing.

Can you replace the TODO comments with descriptions as to why you chose the 
values
you did?  I think that'd make it a bit more clear.

Original comment by dsalli...@gmail.com on 20 May 2009 at 4:39

• Changed state: NeedInfo

[GoogleCodeExporter]

I supposed that i have three memcached server node (node1 ,  node2 , node3),
the node1 has the value of the specific key(for example, 12).
if node1 will be unavailable, for the key[12] must be allocate to node2 or 
node3,
but the nodes of getSequence()'s iterator are all same value(node1).
therefore, the specific key cant not be stored and get.

the following is MemcachedConnection class's addOperation(final String key, 
final 
Operation o) method.

public void addOperation(final String key, final Operation o) {
        MemcachedNode placeIn=null;
        MemcachedNode primary = locator.getPrimary(key);
        if(primary.isActive() || failureMode == FailureMode.Retry) {
            placeIn=primary;
        } else if(failureMode == FailureMode.Cancel) {
            o.cancel();
        } else {
            // Look for another node in sequence that is ready.
            for(Iterator<MemcachedNode> i=locator.getSequence(key);
                placeIn == null && i.hasNext(); ) {
                MemcachedNode n=i.next
(); //!!!!!!!!!!!!!!!!!!!!!!!!! for the specific key, n is always same 
node ,therefore the node being inavailable, i can't get other available node
                if(n.isActive()) {
                    placeIn=n;
                }
            }
            // If we didn't find an active node, queue it in the primary 
node
            // and wait for it to come back online.
            if(placeIn == null) {
                placeIn = primary;
            }
        }

        assert o.isCancelled() || placeIn != null
            : "No node found for key " + key;
        if(placeIn != null) {
            addOperation(placeIn, o);
        } else {
            assert o.isCancelled() : "No not found for "
                + key + " (and not immediately cancelled)";
        }
    }

Original comment by mayd...@gmail.com on 21 May 2009 at 1:11

[GoogleCodeExporter]

see the MemcachedConnection's from 516 line,

Original comment by mayd...@gmail.com on 21 May 2009 at 1:13

[GoogleCodeExporter]

i modified test a little bit, findNode() method's code is coppied from 
MemcachedConnection class's addOperation method's some code.

import java.io.IOException;
import java.net.SocketAddress;
import java.nio.ByteBuffer;
import java.nio.channels.SelectionKey;
import java.nio.channels.SocketChannel;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Iterator;
import java.util.List;

import org.junit.Before;
import org.junit.Test;

import net.spy.memcached.HashAlgorithm;
import net.spy.memcached.KetamaNodeLocator;
import net.spy.memcached.MemcachedNode;
import net.spy.memcached.ops.Operation;
import static junit.framework.Assert.*;

public class KetamaNodeLocatorTest {

    private KetamaNodeLocator ketamaNodeLocator;
    List<MemcachedNode> memcachedNodeList;
    MockMemcachedNode node1 = new MockMemcachedNode("11211");
    MockMemcachedNode node2 = new MockMemcachedNode("11212");
    MockMemcachedNode node3 = new MockMemcachedNode("11213");   

    @Before
    public void setUp() throws Exception {
        memcachedNodeList = new ArrayList<MemcachedNode>();
        memcachedNodeList.add(node1);
        memcachedNodeList.add(node2);
        memcachedNodeList.add(node3);

        ketamaNodeLocator = new KetamaNodeLocator(memcachedNodeList, 
HashAlgorithm.KETAMA_HASH);
    }

    /**
     *  result
    12:2
    13:2
    26:2
    29:2
    41:2
    78:2
    85:2
    86:2
    97:2
    **/

    @Test
    public void getNode() throws Exception {
        for(int k = 0 ; k < 100; k++){
            String result = null;
            int equalCount = 0;
            for(Iterator<MemcachedNode> i=ketamaNodeLocator.getSequence
(String.valueOf(k));i.hasNext(); ) {
                MemcachedNode n=i.next();
                if(n.toString().equals(result)){
                    equalCount++;
                }
                result = n.toString();
            }

            if(equalCount == 2){
                System.out.println(k + ":" + equalCount );
            }
        }
    }

    private String testKey = "12";
    @Test
    public void findNode() throws Exception {
        node1.setActive(false);
        MemcachedNode placeIn = null;
        for(Iterator<MemcachedNode> i= ketamaNodeLocator.getSequence
(testKey);
            placeIn == null && i.hasNext(); ) {

            MemcachedNode n=i.next();
            if(n.isActive()) {
                placeIn=n;
            }
        }

        assertNull(placeIn);//this will be asserted, but placeIn must not 
be null if node2 and node3 are active.

    }
}
class MockMemcachedNode implements MemcachedNode {

    private String socketAddress;
    private boolean active;

    public MockMemcachedNode(String socketAddress){
        this.socketAddress = socketAddress;
    }

    @Override
    public String toString() {
        return socketAddress;
    }

    public SocketAddress getSocketAddress() {
        // TODO Auto-generated method stub
        return new SocketAddress(){
            @Override
            public String toString() {
                return socketAddress;
            }
        };
    }

    public boolean isActive() {
        return active;
    }

    public void setActive(boolean active) {
        this.active = active;
    }

    public void addOp(Operation op) {
        // TODO Auto-generated method stub

    }

    public void connected() {
        // TODO Auto-generated method stub

    }

    public void copyInputQueue() {
        // TODO Auto-generated method stub

    }

    public Collection<Operation> destroyInputQueue() {
        // TODO Auto-generated method stub
        return null;
    }

    public void fillWriteBuffer(boolean optimizeGets) {
        // TODO Auto-generated method stub

    }

    public void fixupOps() {
        // TODO Auto-generated method stub

    }

    public int getBytesRemainingToWrite() {
        // TODO Auto-generated method stub
        return 0;
    }

    public SocketChannel getChannel() {
        // TODO Auto-generated method stub
        return null;
    }

    public Operation getCurrentReadOp() {
        // TODO Auto-generated method stub
        return null;
    }

    public Operation getCurrentWriteOp() {
        // TODO Auto-generated method stub
        return null;
    }

    public ByteBuffer getRbuf() {
        // TODO Auto-generated method stub
        return null;
    }

    public int getReconnectCount() {
        // TODO Auto-generated method stub
        return 0;
    }

    public int getSelectionOps() {
        // TODO Auto-generated method stub
        return 0;
    }

    public SelectionKey getSk() {
        // TODO Auto-generated method stub
        return null;
    }

    public ByteBuffer getWbuf() {
        // TODO Auto-generated method stub
        return null;
    }

    public boolean hasReadOp() {
        // TODO Auto-generated method stub
        return false;
    }

    public boolean hasWriteOp() {
        // TODO Auto-generated method stub
        return false;
    }

    public void reconnecting() {
        // TODO Auto-generated method stub

    }

    public void registerChannel(SocketChannel ch, SelectionKey selectionKey) {
        // TODO Auto-generated method stub

    }

    public Operation removeCurrentReadOp() {
        // TODO Auto-generated method stub
        return null;
    }

    public Operation removeCurrentWriteOp() {
        // TODO Auto-generated method stub
        return null;
    }

    public void setChannel(SocketChannel to) {
        // TODO Auto-generated method stub

    }

    public void setSk(SelectionKey to) {
        // TODO Auto-generated method stub

    }

    public void setupResend() {
        // TODO Auto-generated method stub

    }

    public void transitionWriteItem() {
        // TODO Auto-generated method stub

    }

    public int writeSome() throws IOException {
        // TODO Auto-generated method stub
        return 0;
    }

}

Original comment by mayd...@gmail.com on 21 May 2009 at 1:29

[GoogleCodeExporter]

And TODO comment was auto-generating's result. don't take care. 
MockMemcachedNode is 
not important part in this test

Original comment by mayd...@gmail.com on 21 May 2009 at 5:56

[GoogleCodeExporter]

I think you've provided some info.  I'll need a bit of time to fully understand 
this.
 In the initial version, active was hard-coded to false.  Thank you for the report.

Original comment by dsalli...@gmail.com on 21 May 2009 at 6:07

• Changed state: New

[GoogleCodeExporter]

active's initial value must be true.. that's my mistake..
I hope we will get along well..

Original comment by mayd...@gmail.com on 21 May 2009 at 6:57

[GoogleCodeExporter]

I believe this is a duplicate of 
http://code.google.com/p/spymemcached/issues/detail?id=117

Original comment by kevin.la...@gmail.com on 14 Sep 2010 at 12:56