-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Cleanup usages of stopwatch #16478
Cleanup usages of stopwatch #16478
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! The Stopwatch
changes make sense to me. I left a few suggestions and questions on the usages of synchronized
blocks in ChangeRequestHttpSyncer
.
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHttpSyncer.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
private void sync() | ||
private void sendSyncRequest() | ||
{ | ||
if (!startStopLock.awaitStarted(1, TimeUnit.MILLISECONDS)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much all the current usages of awaitStarted
is preceded by synchronized (startStopLock)
. Curious why this one doesn't have. I see this runs in an executor thread, but I'm wondering if the new synchronized block added below in line 242 should be moved up here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely sure if the startStopLock
is really being used in the correct way in this entire class. Calling various methods on this lock (LifecycleLock
) doesn't require synchronizing on it. So left this as is until we plan to revisit this code.
@@ -410,6 +420,7 @@ private void addNextSyncToWorkQueue() | |||
} | |||
} | |||
|
|||
@GuardedBy("startStopLock") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to just have the lock synchronization in this method directly instead of relying on the different callers to do the right thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fair enough. I guess the only part which really needs the synchronization is the update of the failed attempt count and the stopwatch reset.
Btw, @GuardedBy
forces the callers to do the right thing as it throws a compile-time error if not called from within the proper synchronization.
Co-authored-by: Abhishek Radhakrishnan <abhishek.rb19@gmail.com>
Thanks for the review, @abhishekrb19 ! |
Changes: - Remove synchronized methods from `Stopwatch` - Access stopwatch methods in `ChangeRequestHttpSyncer` inside a lock
Description
The Druid
Stopwatch
currently exposes synchronizedstart()
andstop()
methods but this is not adequate asstart()
may still be called beforestop()
has been called throwing an exceptionThis stopwatch is already running
.Moreover, there doesn't seem to be a typical use case of a
Stopwatch
that requires thread safety in the first place. In Druid,ChangeRequestHttpSyncer
seems to be the only place that may use the stopwatch in an unsafe manner. All other usages simply create a fresh instance in a method.All the usages in
ChangeRequestHttpSyncer
are always accessed inside astartStopLock
. The two places which were not synchronized on thestartStopLock
have been updated to do so.