Allow named tailers to be used concurrently to split work #964 #965
Conversation
peter-lawrey
requested review from
nicktindall,
RobAustin,
JerryShea and
rogersimmons
| @@ -195,6 +201,25 @@ public DocumentContext readingDocument(final boolean includeMetaData) { | |||
| return documentContext; | |||
There was a problem hiding this comment.
Do we need to replicate this check in the named version? or can we remove it from unnamed? it makes it more confusing if they're inconsistent
| else | ||
| indexValue.setValue(index); | ||
| } else if (!indexValue.compareAndSwapValue(this.indexChecker, index)) { | ||
| this.indexChecker = Long.MIN_VALUE; // invalid. |
There was a problem hiding this comment.
Would it be better to give this a constant with a meaningful name? It's a bit hard to follow what the significance of Long.MIN_VALUE is.
| } | ||
| documentContext.close(); | ||
| } | ||
| throw new AssertionError(); |
There was a problem hiding this comment.
Is it possible in cases of high contention that you'd get to the end of the 100 iterations? Probably very unlikely but even if it's a remote possibility we should probably put a meaningful exception/message in here.
|
Apologies for pushing to the branch, but I just added a test to the test class that runs concurrent competing consumer threads and it doesn't seem to work. Perhaps I've missed something, alarmingly the test breaks even for a single consumer. |
| @@ -195,6 +201,25 @@ public DocumentContext readingDocument(final boolean includeMetaData) { | |||
| return documentContext; | |||
| } | |||
|
|
|||
| DocumentContext readingDocumentNamed(final boolean includeMetaData) { | |||
| for (int i = 0; i < 100; i++) { | |||
There was a problem hiding this comment.
Under what conditions would this loop run to completion and throw the AssertionError?
Stopping after 100 iterations looks arbitrary. Is there something more robust we can do here?
It feels that the loop itself should retry indefinitely, and either succeed eventually, or internally detect any genuine error condition and throw from the body
| } | ||
| Jvm.startup().on(ConcurrentNamedTailersTest.class, "Wrote " + numberOfEntries + " entries"); | ||
|
|
||
| final int numReaders = 6; |
There was a problem hiding this comment.
this fails on my machine when numReaders = 1
|
Closing PR in order to close issue - we can re-open as needed |
Previously the behaviour of concurrent named tailers with the same name was undefined and not supported.
This change adds support for splitting work by named tailer by checking whether the index has changed during the call to readingDocument
(changing the index in the table store at the end of readingDocument instead of close() )
I have added a test to show it detects and retries if the index is change while readingDocument is being performed.