Young-Seok Kim has posted comments on this change. Change subject: Introducing Data Replication To AsterixDB ......................................................................
Patch Set 6: (10 comments) Another code review comments. I will continue reviewing. 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 109: ByteBuffer bb = ByteBuffer.allocate(REPLICATION_FUNCATION_SIZE); > Goodbye buffer is sent at the end of every LSM component request to indicat Maybe, this can be just final static bytebuffer object, right? https://asterix-gerrit.ics.uci.edu/#/c/338/6/asterix-replication/src/main/java/org/apache/asterix/replication/management/ReplicationManager.java File asterix-replication/src/main/java/org/apache/asterix/replication/management/ReplicationManager.java: Line 373: IReplicationJob replicationJobPoisonPill = new AsterixReplicationJob(ReplicationJobType.METADATA, poisonPill can be static final object. Line 757: if (responseFunction == ReplicationFunctions.FLUSH_INDEX) { what is this for? why the replica send this responseFunction? Line 767: if (laggingIndexesResponse != null) { What is this for? Where is this information used later? Line 810: clientSocket.read(dataBuffer); How is it guaranteed to get the requested LSN? Is there any chance that the requested replica node may write something other than the requested LSN? Line 862: if (!fileProperties.isLSMComponentFile() && !fileProperties.getNodeId().equals(nodeId)) { Please add comments to explain why this if clause is necessary Line 927: recoveryLogs.add(logRecord); Memory may not be large enough to keep all remote logs. The recoveryLogs should be backed by a file somehow if it exceeds a certain threshold size. Line 930: logManager.log(logRecord); why are logRecords belonging to other nodes sent to ? What am I missing here? Line 1103: closeSockets(); Should it wait for possible incoming replication jobs even if the jobQ size is 0 for now? Line 1153: //read ACK for job commit log Is this server socket used only for getting ack for job commit log? Is there any other messages coming from this server socket? -- 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
