Skip to content
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

Fix 2592 neo4j connection options #2632

Merged
merged 2 commits into from
Jun 11, 2021
Merged

Fix 2592 neo4j connection options #2632

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bac290
fix(neo4j) Adding configuration for liveness check (#2592)
Jun 1, 2021
e86cbdd
fix(neo4j) Change max connection lifetime setting format from hours t…
Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions gms/factories/src/main/java/com/linkedin/gms/factory/common/Neo4jDriverFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,38 @@ public class Neo4jDriverFactory {
@Value("${NEO4J_MAX_CONNECTION_ACQUISITION_TIMEOUT_IN_SECONDS:60}")
private Long neo4jMaxConnectionAcquisitionTimeout;

@Value("${NEO4j_MAX_CONNECTION_LIFETIME_IN_HOURS:1}")
private Long neo4jMaxConnectionLifetime;
// Kept for sake of backwards compatibility. Instead use NEO4j_MAX_CONNECTION_LIFETIME_IN_SECONDS
@Value("${NEO4j_MAX_CONNECTION_LIFETIME_IN_HOURS:#{null}}")
private Long neo4jMaxConnectionLifetimeInHours;

@Value("${NEO4j_MAX_CONNECTION_LIFETIME_IN_SECONDS:3600}")
private Long neo4jMaxConnectionLifetimeInSeconds;

@Value("${NEO4J_MAX_TRANSACTION_RETRY_TIME_IN_SECONDS:30}")
private Long neo4jMaxTransactionRetryTime;

@Value("${NEO4J_CONNECTION_LIVENESS_CHECK_TIMEOUT_IN_SECONDS:-1}")
private Long neo4jConnectionLivenessCheckTimeout;
Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would benefit other folks to give this a reasonable default- I imagine if you're running into liveness issues others may be as well. What value have you found the be successful here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 has worked well under testing conditions, however it stresses Neo4j more. From the docs:

If this option is set too low, an additional network call will be incurred when acquiring a connection, which causes a performance hit.

So I thought, better to have it as the default in the java-driver, i.e disabled, and only tune it when needed.
Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree 0 is a bit aggressive as a default- where you able to find success with a slightly higher timeout? Checking every 5 minutes, for example, seems like a reasonable default. If that doesn't work in your case, I agree -1 seems like a safer default for now until we can get more data points.

Copy link
Contributor Author

@RickardCardell RickardCardell Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Checking every 5 minutes, for example, seems like a reasonable default.

Just to clarify, this is not a keep-alive feature that one would expect, but rather a feature that verifies that the connection is alive upon use.
So not as useful as NEO4j_MAX_CONNECTION_LIFETIME_IN_SECONDS perhaps, but it still adds value, when the connection to Neo4j is flaky or as an extra check to avoid exceptions in the GUI.

If this setting is used and the connection is closed, it will still generate an exception on the GMS, but the connection will be refreshed and the frontend won't notice anything, other than perhaps a delay.

So, yes, I would prefer if -1 is kept as default, since it's more of a power-user setting.


@Bean(name = "neo4jDriver")
protected Driver createInstance() {

Config.ConfigBuilder builder = Config.builder();
builder.withMaxConnectionPoolSize(neo4jMaxConnectionPoolSize);
builder.withConnectionAcquisitionTimeout(neo4jMaxConnectionAcquisitionTimeout, TimeUnit.SECONDS);
builder.withMaxConnectionLifetime(neo4jMaxConnectionLifetime, TimeUnit.HOURS);
builder.withMaxConnectionLifetime(neo4jMaxConnectionLifetime(), TimeUnit.SECONDS);
builder.withMaxTransactionRetryTime(neo4jMaxTransactionRetryTime, TimeUnit.SECONDS);
builder.withConnectionLivenessCheckTimeout(neo4jConnectionLivenessCheckTimeout, TimeUnit.SECONDS);

return GraphDatabase.driver(uri, AuthTokens.basic(username, password), builder.build());
}

private Long neo4jMaxConnectionLifetime() {

// neo4jMaxConnectionLifetimeInHours has precedence over neo4jMaxConnectionLifetimeInSeconds
if (neo4jMaxConnectionLifetimeInHours != null) {
return neo4jMaxConnectionLifetimeInHours * 3600;
}
return neo4jMaxConnectionLifetimeInSeconds;
}
}