Configurable deadlock detector interval for ingester. (resubmit) #1134
Conversation
marqc
requested review from
black-adder,
jpkrohling,
pavolloffay,
vprithvi and
yurishkuro
as code owners
| if atomic.LoadUint64(detector.msgConsumed) == 0 { | ||
| s.panicFunc(-1) | ||
| return // For tests | ||
| if s.interval == 0 { |
There was a problem hiding this comment.
move to head of start() & exit immediately instead?
There was a problem hiding this comment.
Cannot really do that, because without initialization of "allPartitionsDeadlockDetector" API gets broken - and NullPointerExceptions will start to appear.
I little change this code not to check for interval inside goroutine.
Codecov Report
@@ Coverage Diff @@
## master #1134 +/- ##
======================================
Coverage 100% 100%
======================================
Files 144 144
Lines 6822 6839 +17
======================================
+ Hits 6822 6839 +17
Continue to review full report at Codecov.
|
marqc
force-pushed
the
issue-1125
branch
2 times, most recently
from
88191d8
to
c4c4a8d
Compare
Value of 0 disables deadlock_detector. #issue1225 Signed-off-by: Chodor Marek <marek.chodor@grupawp.pl>
There was a problem hiding this comment.
Thanks for staying on it, @marqc. I wonder if there is a simpler solution here. Essentially, when the check interval is zero we want a noop implementation. We can do it explicitly with a separate type, but it might be a lot of churn as we'd need interfaces as well.
Another option is adding a "disabled" flag that will short-circuit all externally called functions to noop.
An even trickier variant of disabled monitor is to return mail from constructor and short-circuit functions based on nil receiver. This might not work if we expose deadlock monitor in fx module.
In all cases we don't want to create any channels. close() functions can exit immediately.
Signed-off-by: Chodor Marek <marek.chodor@grupawp.pl>
|
@yurishkuro I got rid of closed flag, and introduced disabled flag with short circuits as You suggested. Also changed logic to not create unnecessary channels. |
Signed-off-by: Chodor Marek <marek.chodor@grupawp.pl>
Signed-off-by: Chodor Marek <marek.chodor@grupawp.pl>
Signed-off-by: Chodor Marek <marek.chodor@grupawp.pl>
|
@yurishkuro I applied your suggestions |
ghost
assigned 
| w.incrementMsgCount() | ||
| w.incrementAllPartitionMsgCount() | ||
| assert.Zero(t, len(w.closePartitionChannel())) | ||
| w.close() |
There was a problem hiding this comment.
I am not clear what this is testing. Why calling increments should have any effect on the closePartitionChannel?
Resubmit because of github failures today, and previously submitter PR seems broken
Which problem is this PR solving?
Short description of the changes