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.

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

Transaction timeout monitor #9779

Merged
merged 2 commits into from Aug 13, 2017
Merged

Conversation

MishaDemianenko
Copy link
Contributor

Introduce kernel transaction timeout monitor thread,
that will run in the background and will mark timed out transactions
as terminated in addition to existent execution guard checker.

Introduce kernel transaction timeout monitor thread,
that will run in the background and will mark timed out transactions
as terminated in addition to existent execution guard checker.
@tinwelint
Copy link
Member

Perhaps this opens op an opportunity to only do this checking and remove the guarding statement operations "layer"? And increase default rate to 1s or something. Would that be a good idea?

@MishaDemianenko
Copy link
Contributor Author

@tinwelint we can remove only part that actually checks time out but can't remove the part that throwing guarded exception from transaction thread since it's actually that exception that will cause the rollback in most of the cases.

@tinwelint
Copy link
Member

Yes OK. I could imagine a future where this wouldn't be a specific GuardException, but rather a termination with a timeout classification instead. Would be nice to have one unified way of terminating/aborting a transaction. Of course this would not be part of this PR, but food for thought.

@@ -240,6 +240,10 @@
@Description("The maximum time interval of a transaction within which it should be completed.")
public static final Setting<Long> transaction_timeout = setting( "dbms.transaction.timeout", DURATION, String.valueOf( UNSPECIFIED_TIMEOUT ) );

@Description("Configures the time interval between transaction monitor checks. Determines how often " +
"monitor thread will check transaction for timeout.")
public static final Setting<Long> transaction_monitor_check_interval = setting( "dbms.transaction.monitor.check.interval", DURATION, "10s" );
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it hurts to lower this a little, how about 5s max?

@@ -240,6 +240,10 @@
@Description("The maximum time interval of a transaction within which it should be completed.")
public static final Setting<Long> transaction_timeout = setting( "dbms.transaction.timeout", DURATION, String.valueOf( UNSPECIFIED_TIMEOUT ) );

@Description("Configures the time interval between transaction monitor checks. Determines how often " +
"monitor thread will check transaction for timeout.")
public static final Setting<Long> transaction_monitor_check_interval = setting( "dbms.transaction.monitor.check.interval", DURATION, "10s" );
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this setting maybe not allow negative values?
It might also be good to be able to turn this monitor off by setting check interval to zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

duration setting type does not allow negative values already,
do not know what would be the real use case for that but why not....just in case something goes really wrong...will not schedule job if 0 is specified then

Copy link
Contributor

@burqen burqen left a comment

Choose a reason for hiding this comment

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

Simple, clean, small and efficient! 🎉

Do not schedule monitor job if check interval is 0.
@MishaDemianenko
Copy link
Contributor Author

@tinwelint default interval updated to be 5s
@lutovich update to skip job scheduling in case if check interval is 0.

@MishaDemianenko MishaDemianenko merged commit 7206285 into neo4j:3.1 Aug 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants