Skip to content
New issue

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

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

Already on GitHub? # to your account

Construct routing driver from first available address #372

Merged
merged 2 commits into from
May 24, 2017

Conversation

technige
Copy link
Contributor

No description provided.

@technige technige changed the title Added method and test Construct routing driver from first available address May 17, 2017
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.

@technige changes look good. Should this go in 1.3 or 1.4?

* @param authToken authentication to use, see {@link AuthTokens}
* @return a new driver instance
*/
public static Driver routingDriverFromFirstAvailableAddress( List<String> addresses, AuthToken authToken )
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is ok to not add this method because we do not add another overload with just list of addresses?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also vote for a single (long) method to start with.

@@ -592,6 +616,24 @@ public Logger getLog( String name )
return GraphDatabase.driver( boltUri, clusterRule.getDefaultAuthToken(), config );
}

private Driver discoverDriver( List<String> addresses )
{
Logging devNullLogging = new Logging()
Copy link
Contributor

Choose a reason for hiding this comment

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

DevNullLogging.DEV_NULL_LOGGING constant can be used instead

* This is wrapper for the {@link #driver} method that finds the first
* server to respond positively.
*
* @param addresses a list of server addresses for Neo4j instances
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we mention that address is host or host:port without scheme?

@technige
Copy link
Contributor Author

@lutovich This will actually be for 1.4. Good point.

@lutovich lutovich changed the base branch from 1.3 to 1.4 May 19, 2017 13:06
Copy link
Contributor

@zhenlineo zhenlineo left a comment

Choose a reason for hiding this comment

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

I would suggest to keep use URI (with scheme) as it might be confusing that sometimes we require scheme and sometimes we do not.

* @param config user defined configuration
* @return a new driver instance
*/
public static Driver routingDriverFromFirstAvailableAddress( List<String> addresses, AuthToken authToken, Config config )
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do something like

public static Driver routingDriver( List<URI> uris, AuthToken authToken, Config config )
{
    for( URI uri: uris )
    {
        assertRoutingScheme( uri );
        try
        {
            return driver( uri, authToken, config );
        }
        catch( ServiceUnavailableException e )
        {
            // try the next one
        }
    }
    throw new ServiceUnavailableException( "Failed to discover an available server" );
}

private static void assertRoutingScheme( uri )
{
    if( !uri.getScheme().equals("bolt+routing") )
    {
        throw new IllegalArgumentException( "Unsupported scheme " + uri.getScheme() + " for routing driver. Supported scheme is 'bolt+routing'. " );
    }
}

* @param authToken authentication to use, see {@link AuthTokens}
* @return a new driver instance
*/
public static Driver routingDriverFromFirstAvailableAddress( List<String> addresses, AuthToken authToken )
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also vote for a single (long) method to start with.

technige and others added 2 commits May 23, 2017 14:42
 * use shorter name `routingDriver` instead of `routingDriverFromFirstAvailableAddress`
 * no overloads
 * take `Iterable<URI>` of server URIs and verify 'bolt+routing' scheme
 * introduce constants for URIs schemes
@lutovich lutovich force-pushed the 1.3-first-available-address branch from 3da70e9 to 2dab779 Compare May 23, 2017 13:05
@zhenlineo zhenlineo merged commit 5708274 into 1.4 May 24, 2017
@zhenlineo zhenlineo deleted the 1.3-first-available-address branch May 24, 2017 08:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants