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

Actually run the distributed tx expiry task #10297

Merged

Conversation

timw
Copy link
Contributor

@timw timw commented Sep 3, 2024

What does this PR do?

Actually create a timer and schedule the distributed transaction timeout task.
The com.orientechnologies.orient.server.distributed.impl.ODistributedDatabaseImpl#startTxTimeoutTimerTask method creates the timeout task, but never schedules it on a Timer.

Motivation

During heap dump analysis we observed some transactions remaining in the tracking list forever.

Related issues

Additional Notes

Checklist
[x] I have run the build using mvn clean package command
[] My unit tests cover both failure and success scenarios

@timw timw force-pushed the transaction_timer_expiry branch from 9e855e4 to 79d16df Compare September 3, 2024 23:15
Copy link
Member

@tglman tglman left a comment

Choose a reason for hiding this comment

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

This look good, just using the global timer will do!

@@ -651,6 +654,9 @@ public void run() {
checkTxTimeout();
}
};
final long timeout = OGlobalConfiguration.DISTRIBUTED_TX_EXPIRE_TIMEOUT.getValueAsLong();
timer = new Timer("Distributed database tx expiry", true);
timer.scheduleAtFixedRate(txTimeoutTask, timeout / 3, timeout / 3);
Copy link
Member

Choose a reason for hiding this comment

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

We do have actually a global timer in the context so context.schedule should do, just to keep on check the number of threads running.

Probably is also better to execute the operation in the context thread pool, so do ~ context.execute(() -> checkTxTimeout()) because the timer is shared is better that the timer do just timing, I can add this myself later

(and did we really missed that? 🤦 )

@timw timw force-pushed the transaction_timer_expiry branch from 79d16df to f9cd3bd Compare September 5, 2024 09:23
@tglman
Copy link
Member

tglman commented Sep 5, 2024

Perfect, Thanks!

@tglman tglman merged commit 75e3e0f into orientechnologies:3.2.x Sep 5, 2024
4 checks passed
@timw timw deleted the transaction_timer_expiry branch September 10, 2024 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants