Murtadha Hubail has posted comments on this change. Change subject: Introducing Data Replication To AsterixDB ......................................................................
Patch Set 6: (37 comments) Young-Seok, Thanks for the thorough review so far. I have addressed your comments and answered your questions. I also fixed the license header on all the files. Please let me know if anything isn't clear. I will hold on uploading the new patch until I get the rest of your comments. https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplicationEntryPoint.java File asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplicationEntryPoint.java: Line 162: //Note: this is a hack since each node right now maintains its own copy of the cluster configuration. > Who provides the original configuration? When is the copy created? and Wher Right now each NC gets a copy of the cluster configuration file during the binary distribution. The file is static and never modified locally. Line 187: recoveryMgr.checkpoint(true, NON_SHARP_CHECKPOINT_TARGET_LSN); > It will be simpler if we can do checkpoint right after LifeCycleComponentMa Done. Checkpoint in both cases now happen after LifeCycleComponentManager().stopAll(). https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-common/src/main/java/org/apache/asterix/common/config/AsterixReplicationProperties.java File asterix-common/src/main/java/org/apache/asterix/common/config/AsterixReplicationProperties.java: Line 14: */ > This license should be changed. Done Line 173: public Set<String> getNodeReplicas(String nodeId) { > It's better to change the method name as something like getNodeReplicaIds() Did the renaming. This method was changed to just add the node itself and the result of getRemoteReplicasIds(). Line 197: for (int i = nodeIndex - 1; i >= 0; i--) { > Why is the direction of iteration different from the for clause in getRemo This code doesn't exist anymore. Line 208: for (int i = cluster.getNode().size() - 1; i >= 0; i--) { > same comment for this This code doesn't exist anymore. Line 221: public Set<String> getNodeDataReplicas(String nodeId) { > Please help me understand the difference between this method and the method In one of my code iterations getNodeReplicas() excluded the node itself and this one included it. Right now, they are the same. I removed this method and refactored getNodeReplicas to just add the node itself and the result of getRemoteReplicasIds(). https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-common/src/main/java/org/apache/asterix/common/replication/AsterixReplicationJob.java File asterix-common/src/main/java/org/apache/asterix/common/replication/AsterixReplicationJob.java: Line 22: > Please explain the difference between the LSMIndexReplicationJob and Asteri LSMIndexReplicationJob is used for LSM Components only in Hyracks level. AsterixReplicationJob is used for everything else. Currently it is used to transfer indexes metadata files. https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-common/src/main/java/org/apache/asterix/common/replication/IReplicationLifecycleListener.java File asterix-common/src/main/java/org/apache/asterix/common/replication/IReplicationLifecycleListener.java: Line 20: > What's the purpose of having a separate Interface without having any added Removed. https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-common/src/main/java/org/apache/asterix/common/replication/Replica.java File asterix-common/src/main/java/org/apache/asterix/common/replication/Replica.java: Line 14: */ > license should be changed. Done Line 59: String replicaIPAddreess = node.getClusterIp(); > typo: replicaIPAddreess --> replicaIPAddress Done Line 73: output.writeUTF(node.getId()); > why UTF? why not? what do you have against UTF? :) https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-common/src/main/java/org/apache/asterix/common/replication/ReplicaEvent.java File asterix-common/src/main/java/org/apache/asterix/common/replication/ReplicaEvent.java: Line 24: public enum ReplicaEventType { > Let's explain the meansing of each event and when each event can happen. SPLIT: remote replica failed. MERGE: remote replica is rejoining the cluster. SHUTDOWN: remote replica is shutting down normally. https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java File asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java: Line 40: * NodeIdLength(4) > NodeIdLength is first written and then NodeId is written. Done Line 133: buffer.put(nodeId.getBytes(java.nio.charset.StandardCharsets.UTF_8)); > why UTF? why not? I might be in a mood to name my cluster nodes in Arabic :) Line 169: public void writeLogRecord(ByteBuffer buffer, long appendLSN) { > In order to avoid duplicated code, we should create an private method which Done. Refactored as writeLogRecordCommonFields(buffer) and readLogHeader(buffer) and readLogBody((buffer, boolean allocateTupleBuffer). This is because the logic to allocateTupleBuffer or not comes from the log header. Line 204: > From here, please reflect the newly added fields to the LogRecordFormat at These fields are not written to the log record. They are just put in the copy to be sent to the remote replica. Only flush has dataset id which is not new and reflected in the size comments. The reader of this class will be confused with or without the comments :) Line 347: public void formJobTerminateLogRecord(ITransactionContext txnCtx, boolean isCommit) { > formXXXLogRecord() methods can be refactored to avoid duplicated code. Done Line 437: logSize += nodeIdLength; > nodeIdLength can be included in the existing constants such as XXX_SIZE ins This is the variable id string length, not the integer length. The integer length is already added to the constants. Line 463: public int serialize(ByteBuffer buffer) { > Again, this method should be refactored. Done Line 498: public void deserialize(ByteBuffer buffer) { > Again, this should be refactored. Done. Refactored as deserialize(ByteBuffer buffer, boolean remoteRecoveryLog, String localNodeId). The new parameters to avoid allocating tuple buffer if there is no need for it. Line 539: tupleBuffer = ByteBuffer.allocate(newValueSize); > You don't want to allocate buffer for every update log record. It's better Done. This buffer should never be reused since its allocated only for remote recovery logs that belong to the local node. I changed it to be local variable. Line 652: public int getSerializedLogSize() { This was not considering the FLUSH log extra fields. I fixed it as well. https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-replication/src/main/java/org/apache/asterix/replication/functions/AsterixReplicationProtocol.java File asterix-replication/src/main/java/org/apache/asterix/replication/functions/AsterixReplicationProtocol.java: Line 42: public final static int REPLICATION_FUNCATION_SIZE = INTEGER_SIZE; > typo: FUNCATION --> FUNCTION Changed to REPLICATION_REQUEST_TYPE_SIZE. Line 45: public enum ReplicationFunctions { > It's better to change the enum class name into something like ReplicationF Renamed to ReplicationRequestType. Added comments for roles. I changed OK to ACK to better describe its function. ReplicationRequestType: REPLICATE_LOG: txn log replication. REPLICATE_FILE: replicate a file(s). DELETE_FILE: delete a file(s). GET_REPLICA_FILES: used during remote recovery to request lost LSM Components. GET_REPLICA_LOGS: used during remote recovery to request lost txn logs. GET_REPLICA_MAX_LSN: used during remote recovery to initialize the log manager LSN. GET_REPLICA_MIN_LSN: used during remote recovery to specify the low water mark per replica. UPDATE_REPLICA: used to update replica info such as IP Address change. GOODBYE: used to notify replicas that the replication request has been completed. REPLICA_EVENT: used to notify replicas about a remote replica split/merge. LSM_COMPONENT_PROPERTIES: used to send the properties of an LSM Component before its physical files are sent. ACK: used to notify the requesting replica that the request has been completed successfully. FLUSH_INDEX: request remote replica to flush an LSM component. Line 67: dataBuffer = ByteBuffer.allocate(requestSize); > This doesn't work, right? The newly allocated buffer is not visible to the That's right and a great catch. I used to do this locally in the methods. I missed this when I factored out the code here. I changed all of them to return the buffer. Line 74: public static void constructLSMComponentPropertiesRequest(LSMComponentProperties lsmCompProp, ByteBuffer buffer) > All constuctXXXRequest() methods write a given object to a destination buff Done Line 77: DataOutputStream oos = new DataOutputStream(outputStream); > Better be avoiding creating these two object in every call unless this call It is one call per component. The two memory writes could be avoided by using the byte array of the outputStream but the socket calls will need to change to be like this: socketChannel.socket().getOutputStream().write(byte[]); which would eliminate the efficiency of sending bytebuffers. I'm planning to do profiling after the code is merged and other optimizations. This could be a case that I should evaluate. Line 83: buffer = ByteBuffer.allocate(requestSize); > This doesn't work, right? The newly allocated buffer is not visible to the Done Line 93: public static ReplicationFunctions getRequestFunction(SocketChannel socketChannel, ByteBuffer byteBuffer) > better be getRequestType() or getReplicationRequestType() Done. Changed to getRequestType() Line 109: ByteBuffer bb = ByteBuffer.allocate(REPLICATION_FUNCATION_SIZE); > Why this bb and the bb in getOKBuffer() methods are allocated every time? Goodbye buffer is sent at the end of every LSM component request to indicate the end of the request. It cannot be a single reusable buffer since multiple threads might use it unless it is synchronized. I don't think allocating 4 bytes is going to be a problem for that frequency. However, I created a single instance of it in ReplicationManager and ReplicationChannel and reused it with synchronization. Same for OKBuffer. Line 139: requestBuffer.putInt(serializedLog.length); > serializedLog.length means the allocated byte array size, not the limit pos I changed the clone to the following which would eliminate this issue: byte[] serializedLog = new byte[logRecord.getSerializedLog().remaining()]; logRecord.getSerializedLog().get(serializedLog, 0, serializedLog.length); remoteLogsQ.offer(serializedLog); I could've also used logRecord.getSerializedLogSize() but this one is more dynamic in case of changes to the log record fields. Line 151: > same comments as constructLSMComponentPropertiesRequest(). Done https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-replication/src/main/java/org/apache/asterix/replication/storage/LSMComponentProperties.java File asterix-replication/src/main/java/org/apache/asterix/replication/storage/LSMComponentProperties.java: Line 57: } > This can be removed. it is used in the create method below. https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-replication/src/main/java/org/apache/asterix/replication/storage/ReplicaResourcesManager.java File asterix-replication/src/main/java/org/apache/asterix/replication/storage/ReplicaResourcesManager.java: Line 153: > What will happen if the system crashes right after the file is deleted, but Your point is correct if we care about nodes recovering (catch up) after they crash. However, currently failed nodes will perform remote recovery when they return. Line 278: public Set<String> getLaggingReplicaIndexes(String replicaId, long targetLSN) throws IOException { > Can we name it as getLaggingReplicaIndexPaths()? Actually this method is not used anymore. Its usage was replaced by the one below. Removed. Line 297: public HashMap<Long, String> getLaggingReplicaIndexes2(String replicaId, long targetLSN) throws IOException { > I understand the naming is hard. :) That's an ugly name but I changed it to it anyway :) -- To view, visit https://asterix-gerrit.ics.uci.edu/338 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I729fdd1144dbc9ff039b4bc414494860d7553810 Gerrit-PatchSet: 6 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: Young-Seok Kim <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
