abdullah alamoudi has posted comments on this change. Change subject: ASTERIXDB-1917: FLUSH_LSN for disk components is not correctly set ......................................................................
Patch Set 2: > > > > The patch seems nice to me. I'm not sure I entirely > understand > > > the > > > > proposed alternative. If I understand the bug correctly, the > > > issue > > > > here basically is that triggerFlushRequest will eagerly set > the > > > > LSN, which is incorrect because it can and will fail to flush > > > when > > > > a request is already pending. Regardless of whether we > persist > > > the > > > > LSN in the component (which does make more sense to do), or > > > inside > > > > the callback, doesn't the same issue apply? > > > > > > Yes, I think even if we persist the LSN into the memory > > component, > > > the same issue still applies since it is still possible to > > override > > > the component LSN (just as this bug). > > > > > > Moreover, I think it's better to keep the IOOperationCallback > as > > it > > > is, otherwise the flush/merge API would carry some extra > > > information (like LSN in this case) to make things work. > > > > I am not saying that the fix is incorrect. Here is what is > > happening. > > We have a memory component's metadata with the memory component. > > This metadata already has pairs of key value. when we flush, we > > move those key values to the disk component: > > See: LSMBTree.java line 359: > > flushingComponent.getMetadata().copy(component.getMetadata()); > > > > what about the LSN? it is not yet written to the disk component. > > after the flush call returns, we do: > > > > operation.getCallback().afterOperation(LSMOperationType.FLUSH, > > null, newComponent); > > > > If we put the lsn when we schedule the operation inside the > > scheduleFlush() call, then it will be written to the metadata of > > the disk component with the other key value pairs. if the lsn was > > set later in the callback, it wouldn't affect the value that goes > > to disk. > > This solution would work. But the problem is that then we have to > put LSN information inside the LSMIndex (Hyracks codebase), where > the LSN actually belongs to AsterixDB. I think the reason for this > IOOperationCallback is to separate LSN from Hyracks. You are right. -- To view, visit https://asterix-gerrit.ics.uci.edu/1771 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: If438e34f8f612458d81f618eea04c0c72c49a9fe Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Luo Chen <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Luo Chen <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: No
