[#2230] KahaDB dedicated cursor iterator#2231
Conversation
…icatedIterator problem
|
I only glanced at this and haven't reviewed it but it sounds like the name should probably be |
'DedicatedItertor' was following the taxonomy of 'DedicatedCursor'. Are you suggesting a change to only the Iterator now, or go ahead and bite off changing both? |
I forgot about the DedicatedCursor, I would keep both the same so it's clear and if there is no breaking change you could rename both. Isolated i think better describes what's really going on, but if it's going to cause problems we can just keep it as Dedicated |
d500651 to
7a4d037
Compare
|
|
||
| Iterator<Entry<Long, MessageKeys>> iterator(Transaction tx, MessageOrderCursor m) throws IOException{ | ||
| return new MessageOrderIterator(tx,m,this); | ||
| return new MessageOrderIterator(tx,m,this,true); |
There was a problem hiding this comment.
I am trying to remember the original PR, but I feel like we should only need 2 methods and not 4.
Shouldn't we only ever need to pass in a new MessageOrderCursor when we want to be isolated? For some reason the recovery() method was updated to use new MessageOrderCursor() but i don't see why that was changed in the original PR. That method already resets it here
I would think we would only two variations of the iterator...one that tracks state (including the last used keys) and uses the MessageOrderCursor that is part of the definition and one that is completely isolated but maybe I am missing something.
There was a problem hiding this comment.
Or maybe we update the recovery() method to not track the last keys either. Either way, it seems like we should either be isolated or not and the 4 methods includes a weird hybrid option of partial isolation
There was a problem hiding this comment.
I took a look closer and there are 2 spots that should be be using the isolated cursor, the recover() method and also the recoverExpired() methods. Also, both methods have a reset call that can go away, for example:
The reason it doesn't break is those methods are used exclusively for Topics and it doesn't interfere with the subscription recovery as each call to recoverNext() for a sub resets the cursor to the correct offset anyways.
Queues and durable subscriptions exclusively use the recoverNext() methods to keep track of state.
| final boolean trackLastKeys; | ||
|
|
||
| MessageOrderIterator(Transaction tx, MessageOrderCursor m, MessageOrderIndex messageOrderIndex) throws IOException { | ||
| MessageOrderIterator(Transaction tx, MessageOrderCursor m, MessageOrderIndex messageOrderIndex, boolean trackLastKeys) throws IOException { |
There was a problem hiding this comment.
Instead of using a single MessageOrderIterator with lots of args I am wondering if it makes sense to just have two impls.
We could rename MessageOrderIterator to IsolatedMessageOrderIterator that does no tracking (uses a new MessageOrderIndex() and doesn't track keys), and then extend that class with MessageOrderIterator that tracks keys and uses the shared MessageOrderCursor.
This is just an idea I had, I'm not sure if it would make it better or worse without trying it compared to constructor orgs and a single impl
There was a problem hiding this comment.
Latest commit 68eea73 applies these changes
…Iterator instead of overload method pattern
…rIterator and no longer call resetCursorPosition()
2 participants
Validation steps: