Luo Chen has posted comments on this change. Change subject: ASTERIXDB-1917: FLUSH_LSN for disk components is not correctly set ......................................................................
Patch Set 1: > > > 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. -- 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: 1 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
