-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
SeekableStreamSupervisor: Don't enqueue duplicate notices. #13334
Conversation
Similar goal to apache#12018, but more aggressive. Don't enqueue a notice at all if it is equal to one currently in the queue.
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.
Minor suggestions, otherwise looks good.
public boolean equals(Object obj) | ||
{ | ||
// All RunNotices are the same. They are de-duplicated on insertion into the NoticesQueue "notices". | ||
return obj != null && obj.getClass().equals(RunNotice.class); |
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.
Makes sense.
Since the RunNotice has no state, I wonder if we shouldn't just have a singleton like so:
static final RunNotice RUN_NOTICE = new RunNotice()
and just use that wherever needed addNotice(RUN_NOTICE)
.
It would also help clean up the time-based condition in handle
, in that the lastRunTime
can now be a field in RunNotice
rather than in the Supervisor
. Maybe this condition is not even needed now since we are already doing aggressive deduplication in the queue itself.
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 think a singleton makes sense, I can add that tomorrow when fixing the build issues.
However then lastRunTime
couldn't be a field in RunNotice
, since if RunNotice
is a singleton, it's a singleton across all supervisors for all datasources. I'll look into whether it's still needed at all.
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.
Oh, nevermind, it can't be static
singleton since it accesses state of the outer class. I'll just leave it the way it is. I'm not sure if we still need the lastRunTime
, so I'll leave that too.
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.
Ah, of course, we still need to call the actual runInternal
😅
|
||
public T poll(final long timeoutMillis) throws InterruptedException | ||
{ | ||
synchronized (this) { |
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.
Q: Is this preferred over marking the method itself synchronized
? Synchronizing the method would have the added benefit of letting callers know that it is a safe method.
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.
Hmm. I think either one is fine. I tend to prefer synchronized (this)
in code that I write, but not for any important reason.
Build failing because LGTM doesn't like LinkedList 😛 |
public class NoticesQueue<T> | ||
{ | ||
@GuardedBy("this") | ||
private final LinkedList<T> queue = new LinkedList<>(); |
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.
If we don't want a high-performance queue, we could also consider using LinkedHashSet
to combine both queue
and set
. remove
would use the iterator
to remove first element. limiting size can be handled by us if needed.
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'll try this out.
Doh. I'll look into this tomorrow. |
...ng-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/NoticesQueueTest.java
Outdated
Show resolved
Hide resolved
…ord/supervisor/NoticesQueueTest.java Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>
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.
LGTM
Similar goal to #12018, but more aggressive. Don't enqueue a notice at all if it is equal to one currently in the queue. (The prior patch #12018 only considered the head of the queue, not the entire queue.)