abdullah alamoudi has posted comments on this change. Change subject: Change the API for writing and reading metadata pages ......................................................................
Patch Set 5: (8 comments) https://asterix-gerrit.ics.uci.edu/#/c/1396/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/AbstractLSMIOOperationCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/AbstractLSMIOOperationCallback.java: Line 101: ((IMetadataPageManager) treeIndex.getPageManager()).put(LSN_KEY, new MutableArrayValueReference(lsn)); > Why's the cast necessary? Shouldn't we just change it in ITreeIndex? Currently, this cast is here because not every tree can write metadata to their pages. For example, in memory trees can only write free pages but no additional metadata. Line 106: ((IMetadataPageManager) treeIndex.getPageManager()).get(LSN_KEY, pointable); > Can we make the -1L a static final long with some sort of name? Done https://asterix-gerrit.ics.uci.edu/#/c/1396/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java: Line 394: destTuple.setFieldCount(fieldCnt); > What's this about? I made a change to one API which is the ITreeIndexTupleReference This IMO had an interesting design where it had: public byte[] getFieldData(int fIdx); public void resetByTupleOffset(ByteBuffer buf, int tupleStartOffset); This made it less flexible such that we had to always have a ByteBuffer underneath. This will be justified if for some reason we need to access direct buffers but we don't. and even if we do that, then we would need to change the getFieldData method to return a ByteBuffer. What I did was to change public void resetByTupleOffset(ByteBuffer buf, int tupleStartOffset); to public void resetByTupleOffset(byte[] buf, int tupleStartOffset); so it can be used with a wider set of classes. https://asterix-gerrit.ics.uci.edu/#/c/1396/5/asterixdb/asterix-replication/src/main/java/org/apache/asterix/replication/management/ReplicationChannel.java File asterixdb/asterix-replication/src/main/java/org/apache/asterix/replication/management/ReplicationChannel.java: Line 345: String componentId = LSMComponentProperties.getLSMComponentID(afp.getFilePath()); > Again, why introduce magic numbers where static final variables were? because what if we have another negative value. the previous code will break. https://asterix-gerrit.ics.uci.edu/#/c/1396/5/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/ITreeIndexFrame.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/ITreeIndexFrame.java: Line 34: public static class Constants { > Why's this moved to here? The way it was previously defined was not the best way. we have different definitions for some of those values even through they are shared among all kinds of frames. For example, level was defined in two different classes with the same value. if one of them changed and not the other, things would break in an interesting way. What I did was separate the definitions in different places hierarchically. ITreeIndexFrame has all the stuff that are available in all frames. https://asterix-gerrit.ics.uci.edu/#/c/1396/5/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/frames/LIFOMetaDataFrame.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/frames/LIFOMetaDataFrame.java: Line 36: protected static final int MAX_PAGE_OFFSET = ITreeIndexFrame.Constants.RESERVED_HEADER_SIZE; > It would be nice to still have the comments about the running length on eac All the IDEs provide a hover mechanism which I think is sufficient. Otherwise, whenever a new field is introduced, we will need to update all of them in multiple classes and there doesn't seem to be a lot of value for the numbers. Line 273: int offset = getTupleStart(index); > Why 2? I am adding a comment that display the layout of the frame. basically, at the beginning, we have all predefined fields, then we have kv written as key length, key, value length, value. and starting from the end, we have the free pages. the 2 here is for the two lengths values each having 4 bytes. https://asterix-gerrit.ics.uci.edu/#/c/1396/5/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/test/java/org/apache/hyracks/storage/am/common/frames/LIFOMetadataFrameTest.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/test/java/org/apache/hyracks/storage/am/common/frames/LIFOMetadataFrameTest.java: Line 45: + testKey.getLength()); > 52? just arbitrary. -- To view, visit https://asterix-gerrit.ics.uci.edu/1396 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iadad522ab5568677aa816c74fc1d63acad505380 Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
