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

Improve tablet server batch reader iterator time tracking code #4927

Open
wants to merge 2 commits into
base: 3.1
Choose a base branch
from

Conversation

DomGarguilo
Copy link
Member

This pull request introduces the CountDownTimer class to replace the existing Timer class for better handling of timeout and retry logic in the TabletServerBatchReaderIterator class. The most significant changes include the integration of CountDownTimer in various methods and the addition of new methods to the CountDownTimer class to support the new functionality.

Integration of CountDownTimer:

  • TabletServerBatchReaderIterator.java: Replaced Timer with CountDownTimer to simplify handling of retry timeouts and activity tracking in methods such as doLookups, rebinToScanServers, and TimeoutTracker class.

Enhancements to CountDownTimer class:

  • CountDownTimer.java: Added restart method to reset the timer and timeLeft method to get the remaining time as a Duration.

@DomGarguilo DomGarguilo added this to the 3.1.0 milestone Sep 27, 2024
@DomGarguilo DomGarguilo self-assigned this Sep 27, 2024
@dlmarion
Copy link
Contributor

dlmarion commented Oct 2, 2024

I think I missed the initial inclusion of the CountDownTimer class. I went looking to see if an implementation already existed that we could just use and not have to maintain ourselves. I didn't find one, but I found three implementations of a StopWatch. One StopWatch is in our codebase and it appears unused and a candidate for removal, Commons-Lang has one, and Guava does too. A timer is just checking to see if some amount of time has passed. I'm wondering if we should leverage an existing StopWatch implementation inside of the CountDownTimer to reduce the amount of code that we have to maintain and test.

@ctubbsii
Copy link
Member

ctubbsii commented Oct 2, 2024

I think it's worth looking into one of the existing libraries to use. But, we'd need to be careful it's fast and efficient, and not something that is going to be slow by creating unnecessary Duration objects to do simple arithmetic operations on nanoTime.

@DomGarguilo
Copy link
Member Author

Yea I think its worth looking into as well. Maybe we can open a new ticket to remove the StopWatch class thats in our codebase and look into the existing timing utils.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants