Murtadha Hubail has posted comments on this change. Change subject: Introducing Data Replication To AsterixDB ......................................................................
Patch Set 6: (10 comments) 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); > Maybe, this can be just final static bytebuffer object, right? It is final static bytebuffer in ReplicationManager and ReplicationChannel. 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. Done Line 757: if (responseFunction == ReplicationFunctions.FLUSH_INDEX) { > what is this for? why the replica send this responseFunction? This is part of the protocol. When IndexFlsuhRequest is sent to a replica, it responds back by the same type of request.This is done to reuse writeGetReplicaIndexFlushRequest instead of introducing a new request type since both the request and the response use the same type of serialized object (ReplicaIndexFlushRequest). Line 767: if (laggingIndexesResponse != null) { > What is this for? Where is this information used later? This is the replica response to the flush index request. The response contains which indexes it flushed and which it didn't. It is used below to update the LSNMap for the replica indexes that were not flushed. Line 810: clientSocket.read(dataBuffer); > How is it guaranteed to get the requested LSN? Is there any chance that the This is part of the protocol. It will either write back the correct response (just the LSN) or an exception will be thrown. Line 862: if (!fileProperties.isLSMComponentFile() && !fileProperties.getNodeId().equals(nodeId)) { > Please add comments to explain why this if clause is necessary Done. Just like the comment below says, this if statement means that the received file is a .metadata file of an index that belongs to a remote replica. In this case, we need to create an LSN map for it as if it was received during normal operation and not during recovery. Line 927: recoveryLogs.add(logRecord); > Memory may not be large enough to keep all remote logs. The recoveryLogs sh This is a common issue in here and in the current local recovery. The issue is described in JIRA ASTERIXDB-969. I will be adding a way to serialize a set of logs to a temporary file that should be read sequentially. Line 930: logManager.log(logRecord); > why are logRecords belonging to other nodes sent to ? What am I missing her When a node recovers, it recovers its data as well as the replicated data of its remote replicas. Therefore, the logs received that don't belong to the local replica should be relogged incase a remote replica requests them later. Line 1103: closeSockets(); > Should it wait for possible incoming replication jobs even if the jobQ size why leave a connection open when it is not needed? Those async jobs happen only index creation/drop/flushes. The cost of establishing a new connection on that frequency wont be a problem. Line 1153: //read ACK for job commit log > Is this server socket used only for getting ack for job commit log? Is ther This is not a server socket nor an independent socket. This is a socket that belongs to TxnLogsReplicator thread which is the only user of this private class. The InputStream of every socket there is monitored by a dedicated thread to process received job commit ACKs. It's just the name of the variable that need to be changed. -- 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
