Change in asterixdb[master]: Introducing Data Replication To AsterixDB

["Young-Seok Kim (Code Review)" <do...@asterixdb.incubator.apache.org>]

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 <hu...@gmail.com>
Gerrit-Reviewer: Ian Maxon <im...@apache.org>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Murtadha Hubail <hu...@gmail.com>
Gerrit-Reviewer: Yingyi Bu <bu...@gmail.com>
Gerrit-Reviewer: Young-Seok Kim <ki...@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <ba...@gmail.com>
Gerrit-HasComments: Yes