[JCBC-545] Couchbase Connection leaks to a single node in the cluster Created: 05/Sep/14  Updated: 15/Sep/14  Due: 14/Sep/14  Resolved: 15/Sep/14

Status: Resolved
Project: Couchbase Java Client
Component/s: None
Affects Version/s: 1.4.2
Fix Version/s: 1.4.5
Security Level: Public

Type: Bug Priority: Critical
Reporter: Justin Michaels Assignee: Michael Nitschinger
Resolution: Fixed Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   
We are observing 2x the number of connections open on a single node in a given Couchbase cluster. All connections are on port 11210.

The development team believes they have identified a bug that creates this behavor during a cluster change and a new map being distributed to the Couchbase clients. Below is their observation.
... there in the list to randomize the nodes, but there’s a bug in the constructor of Couchbase SDK class com.couchbase.client.vbucket.provider.BucketConfigurationProvider

Here’s the code:

  public BucketConfigurationProvider(final List<URI> seedNodes,
    final String bucket, final String password,
    final CouchbaseConnectionFactory connectionFactory) {
    config = new AtomicReference<Bucket>();
    configurationParser = new ConfigurationParserJSON();
    httpProvider = new AtomicReference<ConfigurationProviderHTTP>(
      new ConfigurationProviderHTTP(seedNodes, bucket, password)
    );
    refreshingHttp = new AtomicBoolean(false);
    pollingBinary = new AtomicBoolean(false);
    observers = Collections.synchronizedList(new ArrayList<Reconfigurable>());
    binaryConnection = new AtomicReference<CouchbaseConnection>();

    this.seedNodes = Collections.synchronizedList(new ArrayList<URI>(seedNodes));
    this.bucket = bucket;
    this.password = password;
    this.connectionFactory = connectionFactory;
    potentiallyRandomizeNodeList(seedNodes);

    disableCarrierBootstrap = Boolean.parseBoolean(
      CouchbaseProperties.getProperty("disableCarrierBootstrap", "false"));
    disableHttpBootstrap = Boolean.parseBoolean(
      CouchbaseProperties.getProperty("disableHttpBootstrap", "false"));
  }


The potentiallyRandomizeNodeList method will randomize the list passed to it and in fact, in our clients we ARE randomizing the two-element list we are passing.
Unfortunately, the wrong list is being passed to this method. The code should be randomizing the list stored as a member variable.
The line should read
potentiallyRandomizeNodeList(this.seedNodes);

because four lines above a completely new list is constructed by making a shallow copy of the input argument, seedNodes.

The list later used to get the configuration information is always used ordered and that’s why all our management connections are to the same node, the first one in our list.

I found this in v1.4.2 of the client, but v1.4.4 seems to have the same code so I don’t think it’s been fixed yet.

 Comments   
Comment by Dan Douglas [ 06/Sep/14 ]
I found this issue.

The symptom is that the configuration information is always obtained from the first node in seed list passed and the shuffle is not done.

The line that does the shuffle in the constructor needs to reference the seedNodes member variable, not the argument. Needs to add "this." like so

potentiallyRandomizeNodeList(this.seedNodes);






[JCBC-525] Add support for full JSON compat docs & common flags Created: 22/Aug/14  Updated: 15/Sep/14  Resolved: 15/Sep/14

Status: Resolved
Project: Couchbase Java Client
Component/s: None
Affects Version/s: None
Fix Version/s: 2.0-beta2
Security Level: Public

Type: Improvement Priority: Major
Reporter: Michael Nitschinger Assignee: Michael Nitschinger
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified





[JCBC-481] Integration Tests hang up after 85% progress Created: 25/Jun/14  Updated: 15/Sep/14  Resolved: 15/Sep/14

Status: Resolved
Project: Couchbase Java Client
Component/s: None
Affects Version/s: None
Fix Version/s: 2.0-dp3
Security Level: Public

Type: Bug Priority: Test Blocker
Reporter: Deepti Dawar Assignee: Michael Nitschinger
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Dependency

 Description   
[root@pomelo-11017 couchbase-java-client]# ./gradlew integrationTest
:compileJava UP-TO-DATE
:processResources UP-TO-DATE
:classes UP-TO-DATE
:compileIntegrationJava UP-TO-DATE
:processIntegrationResources UP-TO-DATE
:integrationClasses UP-TO-DATE
> Building 85% > :integrationTest

 Comments   
Comment by Wei-Li Liu [ 18/Aug/14 ]
The process hangs at 85% due to view query test.
Just runs 2.0.0-dp3 integration test against the new beta2 build, and all the tests completes.
Comment by Deepti Dawar [ 21/Aug/14 ]
Following error appears with latest DP3 -

[root@pomelo-11017 couchbase-java-client]# ./gradlew integrationTest
:compileJava UP-TO-DATE
:processResources UP-TO-DATE
:classes UP-TO-DATE
:compileIntegrationJava
warning: [options] bootstrap class path not set in conjunction with -source 1.6
1 warning
:processIntegrationResources
:integrationClasses
:integrationTest

com.couchbase.client.java.BinaryTest > classMethod FAILED
    com.couchbase.client.core.config.ConfigurationException
        Caused by: java.lang.IllegalStateException
            Caused by: rx.exceptions.OnErrorThrowable$OnNextValue

com.couchbase.client.java.BinaryTest > classMethod FAILED
    java.util.NoSuchElementException

com.couchbase.client.java.DesignDocumentTest > classMethod FAILED
    com.couchbase.client.core.config.ConfigurationException
        Caused by: java.lang.IllegalStateException
            Caused by: rx.exceptions.OnErrorThrowable$OnNextValue

com.couchbase.client.java.DesignDocumentTest > classMethod FAILED
    java.util.NoSuchElementException

4 tests completed, 4 failed
:integrationTest FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':integrationTest'.
> There were failing tests. See the report at: file:///root/couchbase/couchbase-java-client/build/reports/tests/index.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 22.09 secs
Comment by Michael Nitschinger [ 21/Aug/14 ]
Does it also happen on master?
Comment by Wei-Li Liu [ 21/Aug/14 ]
I test it on master and only see JCBC-479




[JCBC-551] Implement blocking API around async one Created: 11/Sep/14  Updated: 15/Sep/14  Resolved: 15/Sep/14

Status: Resolved
Project: Couchbase Java Client
Component/s: None
Affects Version/s: None
Fix Version/s: 2.0-beta2
Security Level: Public

Type: Task Priority: Blocker
Reporter: Michael Nitschinger Assignee: Michael Nitschinger
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified





[JCBC-523] Add support for memcached buckets Created: 22/Aug/14  Updated: 15/Sep/14  Resolved: 15/Sep/14

Status: Resolved
Project: Couchbase Java Client
Component/s: None
Affects Version/s: None
Fix Version/s: 2.0-beta2
Security Level: Public

Type: Improvement Priority: Major
Reporter: Michael Nitschinger Assignee: Sergey Avseyev
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Comments   
Comment by Matt Ingenthron [ 13/Sep/14 ]
Sergey: I believe this is done. Can you verify?
Comment by Sergey Avseyev [ 15/Sep/14 ]
Verified, memcache buckets supported with exception of APIs which are not available there (like locking)




[JCBC-228] a no-args constructor and an init method are needed Created: 31/Jan/13  Updated: 15/Sep/14  Resolved: 15/Sep/14

Status: Resolved
Project: Couchbase Java Client
Component/s: Core
Affects Version/s: 1.1.0, 1.1.1
Fix Version/s: 2.0.0
Security Level: Public

Type: New Feature Priority: Major
Reporter: Matt Ingenthron Assignee: Michael Nitschinger
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   
Currently, the CouchbaseClient does a number of things that are not so pretty, like spinning up a thread from it's ctor. It would be better to add an additional, optional no-args constructor which expects an init method to be called with the other params needed, start thread, etc.

 Comments   
Comment by Michael Nitschinger [ 31/Jan/13 ]
Assigning towards 1.1.2 but we can defer it to 1.1.3 as well.
Comment by Michael Nitschinger [ 19/Dec/13 ]
We'll have something like this in 2.0
Comment by Matt Ingenthron [ 11/Sep/14 ]
Sergey, I think this is true now in 2.0, can you verify and close this if so?
Comment by Sergey Avseyev [ 12/Sep/14 ]
Yes, we have static method create() which does not have any arguments, but it is just provides defaults to full constructor, which does blocking connection anyway.

We might extract connect() method to the public interface, and modify static constructors somehow they will allow to do at least asynchronous connection. I did some sketch here (it just extracts connect() method, but still call it from constructor) http://review.couchbase.org/41397

The patch is not meant to merged, just demonstrate my question.

Matt, could you expand a little bit what we need to achieve?

Does it spawn thread in constructor? No
Does it do blocking connection? Yes
Comment by Matt Ingenthron [ 12/Sep/14 ]
Assigning this to Michael as he's the architecture owner here.

The reason I filed this way back at -228 was that in general, a recommendation with Java is to not do anything requiring IO or threads in the ctor and separate out the actions of setting things up in a separate method. This is often helpful in some frameworks with DI, etc. Imagine for a moment that you throw an IOException from the ctor(). Chances are whatever created that object inline isn't ready to handle that exception right there. However, calling .init() on it and seeing an explicit throws IOException gives the app developer a cleaner way of abstracting the set up of the things and the initializing of the things.

http://www.javapractices.com/topic/TopicAction.do?Id=254

All of these things change over time though, so I trust Michael to pick a solution he can live with.
Comment by Sergey Avseyev [ 12/Sep/14 ]
Michael, I also think that current implementation is good. Abandoned my patch. Please resolve the ticket, or reassign back to me
Comment by Michael Nitschinger [ 15/Sep/14 ]
Yes, we have static factory methods now and should be good on that.

This was actually a relict out of changing the 1.* SDK when we didn't know that 2.0 was a breaking change. So I'm going to close this out.
Comment by Michael Nitschinger [ 15/Sep/14 ]
Requirements have changed since 2.0 is a complete rewrite its not needed anymore




[JCBC-532] Implement Common Flags Created: 27/Aug/14  Updated: 11/Sep/14  Resolved: 11/Sep/14

Status: Resolved
Project: Couchbase Java Client
Component/s: Core
Affects Version/s: None
Fix Version/s: 2.0-beta2
Security Level: Public

Type: Task Priority: Major
Reporter: Brett Lawson Assignee: Michael Nitschinger
Resolution: Duplicate Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   
Implement the Common Flags formatting as defined by the specification.

 Comments   
Comment by Michael Nitschinger [ 11/Sep/14 ]
dups 525




[JCBC-384] couchbase java libs should use log4j or slf4j not JUL Created: 11/Sep/13  Updated: 11/Sep/14  Resolved: 11/Sep/14

Status: Resolved
Project: Couchbase Java Client
Component/s: Dependencies
Affects Version/s: 1.4.0
Fix Version/s: 2.0-dp3
Security Level: Public

Type: Bug Priority: Major
Reporter: Timothy Ehlers Assignee: Michael Nitschinger
Resolution: Fixed Votes: 2
Labels: usability
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   
log4j has more appends and is widely used, it is also more enterprise friendly thanks to the SocketAppender.

 Comments   
Comment by Timothy Ehlers [ 11/Sep/13 ]
A dependency you pull in uses log4j

net/spy/memcached/compat/log/Log4JLogger.java:import org.apache.log4j.Logger;

Then the couchbase lib uses JUL, this seems counter production
com/couchbase/client/CouchbaseClient.java:import java.util.logging.Logger;
Comment by Matt Ingenthron [ 27/Nov/13 ]
Note that we manage/maintain the net.spy.memcached project as well.

We have support for SLF4J, Log4J, and JUL. Because of how things are packaged, all dependencies are described via maven, but they're not all pulled in.
Comment by Matt Ingenthron [ 27/Nov/13 ]
Michael: do you think we can make our packaging better in the 2.0 client?
Comment by Michael Nitschinger [ 28/Nov/13 ]
That's right since 1.2 you can use slf4j. 2.0 will be slf4j only like most other projects out there, only shipping with the binding and you plug in your own impl.
Comment by Matt Ingenthron [ 11/Sep/14 ]
Michael: I think this is a resolved as of 2.0, right?
Comment by Michael Nitschinger [ 11/Sep/14 ]
it is!




[JCBC-540] Enable SDK 1.x & 2.x to be deployed side by side in a single JVM. Consider renaming couchbase-client to java-client Created: 01/Sep/14  Updated: 09/Sep/14  Resolved: 09/Sep/14

Status: Resolved
Project: Couchbase Java Client
Component/s: None
Affects Version/s: None
Fix Version/s: 2.0-beta2
Security Level: Public

Type: Improvement Priority: Major
Reporter: Michael Nitschinger Assignee: Michael Nitschinger
Resolution: Fixed Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Comments   
Comment by Dan Douglas [ 02/Sep/14 ]
Thanks for considering this.

Renaming the artifact is a start. However, the package names should be different, too, to make sure that the class namespace has no clashes.

The goal should be coexistence of both 1.4 SDK and 2.0 SDK clients in the same JVM.

Motivation is Couchbase use by large, distributed teams that will not all migrate in lockstep, yet contribute code that runs in the same JVM.

Lack of separation at both Maven artifact AND class namespace level will impede migration to CB Java 2.0 SDK.
Comment by Michael Nitschinger [ 02/Sep/14 ]
Hm, they both have com.couchbase.client in common, but the new one is now under com.couchbase.client.java and the core is com.couchbase.client.core

Is that sufficient or would the new one be completely different and not under com.couchbase.client at all?
Comment by Michael Nitschinger [ 02/Sep/14 ]
I just tried adding both as dependencies (I locally renamed the new one to java-client and pushed a new artifact) and it seems to work nicely:

    public static void main(String... args) throws Exception {

        CouchbaseClient client = new CouchbaseClient(
            Arrays.asList(URI.create("http://127.0.0.1:8091/pools")),
            "default",
            ""
        );

        Cluster cluster = CouchbaseCluster.create();
        Bucket bucket = cluster.openBucket().toBlocking().single();

        client.set("id", "value1234").get();
        System.out.println(bucket.get("id", LegacyDocument.class).toBlocking().single());

    }
Comment by Dan Douglas [ 02/Sep/14 ]
Doesn't have to be completely different.... just as long as there are no clashes in ANY of the class names.

Different prefixes are ok.

So as long as the SDK 2.0 packages always start with

com.couchbase.client.java
com.couchbase.client.core
com.couchbase.client.deps

and the SDK 1.4 maintenance never adds those paractular packages, it should be ok.

So far, the 1.4 version uses these packages...

com.couchbase.client
com.couchbase.client.clustermanager
com.couchbase.client.http
com.couchbase.client.internal
com.couchbase.client.protocol
com.couchbase.client.vbucket

It seems a little ripe for error, however. Should someone maintaining 1.4 ever forget and slip in .java, .core or .deps ... but like you said, so far so good.

Taking a step back, though, since the 2.0 SDK doesn't actaully have a "client" class (uses Cluster and Bucket objects instead) and the use of "java" seems somewhat superfluous here, perhaps you'd consider dropping "java" and changing the 2.0 SDK packages to

com.couchbase.sdk

or

com.couchbase.jsdk

or

com.couchbase.jcbc

or

something else....



Just a thought. What you have works, although it seems a little subtle and open to creating confusion.

There's precedent for changing package/artifact names together, for example Apache Commons Lang 3 (http://commons.apache.org/proper/commons-lang/javadocs/api-3.3.2/)
Comment by Cihan Biyikoglu [ 02/Sep/14 ]
added the goal to the title for clarity: side by side deployment of 1.x and 2.x SDKs in a JVM.
Comment by Michael Nitschinger [ 04/Sep/14 ]
I'm actually very open to rename the artifact to java-client (http://review.couchbase.org/#/c/41194/), but I'm not sure its a good idea to rename the namespace. Btw "java" is in there to align with "core" for "core-io" and also in the future for other language bindings like "scala" and so forth.

Since the 1.* branch is mostly in bugfix mode right now, I don't think there will be much room for conflict, since all classes added by the new one are under one level deeper and the only thing we need to make sure is not add a "java" or "core" namespace in the current stable SDK.

Would that make sense?
Comment by Dan Douglas [ 04/Sep/14 ]
Michael, you're the man. If you think it's going to be ok to just rename the artifact, then I'll agree with you. If someone introduces a clash later in a "fix", it'll just be a bad fix that will need to be redone. Perhaps you can introduce something in your static analysis or automated test suite to catch that.
Comment by Michael Nitschinger [ 09/Sep/14 ]
Merged!




Generated at Mon Sep 15 21:36:06 CDT 2014 using JIRA 5.2.4#845-sha1:c9f4cc41abe72fb236945343a1f485c2c844dac9.