Fix 2592 neo4j connection options

[RickardCardell]

Makes it possible for a user to fix the issue described in #2592
This change might affect some users since a property got a new name.

I added another Neo4j connection property and changed the name of an existing property.
I changed the name of NEO4j_MAX_CONNECTION_LIFETIME_IN_HOURS since hours isn't granular enough and that seconds are used by all the other connection properties.

New: NEO4J_CONNECTION_LIVENESS_CHECK_TIMEOUT_IN_SECONDS
Changed:
old name: NEO4j_MAX_CONNECTION_LIFETIME_IN_HOURS
new name: NEO4j_MAX_CONNECTION_LIFETIME_IN_SECONDS

Checklist

• The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• Links to related issues (if applicable)
• Tests for the changes have been added/updated (if applicable)
• Docs related to the changes have been added/updated (if applicable)

[gabe-lyons]

in order to not break anyone who may already be providing this value, should we accept both but default to using NEO4j_MAX_CONNECTION_LIFETIME_IN_SECONDS unless NEO4j_MAX_CONNECTION_LIFETIME_IN_HOURS is set elsewhere?

[RickardCardell]

I can look into it. That approach would be more customer oriented.

[RickardCardell]

I've done another attempt, and has now kept the old setting. It will take precedence of the new setting so that current users won't have any issues.

[gabe-lyons]

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?

[RickardCardell]

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?

[gabe-lyons]

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.

[RickardCardell]

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.

[gabe-lyons]

lgtm- thanks @RickardCardell

[shirshanka]

LGTM. Thanks @RickardCardell!