Skip to content

Update routing according to specification #314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Feb 3, 2017
Merged

Update routing according to specification #314

merged 4 commits into from
Feb 3, 2017

Conversation

@zhenlineo
Copy link
Contributor

@zhenlineo zhenlineo commented Jan 30, 2017

No description provided.

@zhenlineo zhenlineo force-pushed the 1.1-fix-routing branch 2 times, most recently from 2a9469a to a332bdf Compare January 30, 2017 15:49
lutovich
lutovich suggested changes Feb 2, 2017
View reviewed changes
Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhenlineo review round completed. Made some comments.

driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterComposition.java Outdated
this.routers.addAll( routers );
}

public boolean isValid()
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can name this method #hasWriters() to be less ambiguous?

driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterComposition.java Outdated
return !routers.isEmpty() && !writers.isEmpty();
return !writers.isEmpty();
}
public boolean isIllegalResponse()
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote for #hasRoutersAndReaders() name to be more explicit

driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterComposition.java Outdated
}
try

final ClusterComposition result;
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declaration can be joined with the assignment

driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterCompositionProvider.java Outdated
{
GetClusterCompositionResponse getClusterComposition( Connection connection );

class Default implements ClusterCompositionProvider
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this implementation to a separate file, I think it deserves this :)

driver/src/main/java/org/neo4j/driver/v1/exceptions/ProtocolException.java Outdated
*/
public class ProtocolException extends Neo4jException
{
private static String CODE = "Protocol violation: ";
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field should be final

driver/src/test/java/org/neo4j/driver/internal/cluster/ClusterCompositionUtil.java Outdated

import static java.util.Arrays.asList;

public class ClusterCompositionUtil
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably good to make this class final and add a private constructor

driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java
events.registerHandler( StubConnectionPool.EventSink.class, new StubConnectionPool.EventSink.Adapter()
{
// given & when
final AtomicInteger ensureRoutingCounter = new AtomicInteger( 0 );
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using Mockito#spy() instead of atomic integers and runnables? Invocations could then be verified as with regular mockito mocks.

driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java Outdated
public class RediscoveryTest
{

private static GetClusterCompositionResponse.Success Success( ClusterComposition cluster )
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Methods Success\Failure should be named success\failure. Would it be valuable to move them to GetClusterCompositionResponse?

driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java Outdated
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
import sun.reflect.generics.reflectiveObjects.NotImplementedException;
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably not use this exception. Prefer java.lang.UnsupportedOperationException

driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java Outdated
public void removeRouter( BoltServerAddress toRemove )
{
removedRouters.add( toRemove );
// throw new UnsupportedOperationException( "Should never remove any router from routing table" );
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this comment be removed?

lutovich
lutovich approved these changes Feb 2, 2017
View reviewed changes
Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhenlineo changes look good. Made couple minor comments about naming.

...in/java/org/neo4j/driver/internal/cluster/GetServersProcedureClusterCompositionProvider.java

import static java.lang.String.format;

public class GetServersProcedureClusterCompositionProvider implements ClusterCompositionProvider
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about ClusterCompositionProviderImpl name for this class?

Copy link
Contributor Author

@zhenlineo zhenlineo Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this long name better, as it allows us to have other providers such as FileClusterCompositionProvider or GetServers2ProcedureClusterCompositionProvider

driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java Outdated
{

private static GetClusterCompositionResponse.Success Success( ClusterComposition cluster )
private static ClusterCompositionResponse.Success Success( ClusterComposition cluster )
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method should be named success

driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java Outdated
}

private static GetClusterCompositionResponse.Failure Failure( RuntimeException e )
private static ClusterCompositionResponse.Failure Failure( RuntimeException e )
Copy link
Contributor

@lutovich lutovich Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method should be named failure

@lutovich lutovich merged commit 5d0403a into neo4j:1.1 Feb 3, 2017
@lutovich lutovich deleted the 1.1-fix-routing branch February 3, 2017 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lutovich lutovich lutovich approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zhenlineo @lutovich