sashapolo commented on a change in pull request #218: URL: https://github.com/apache/ignite-3/pull/218#discussion_r672261070
########## File path: modules/raft/src/test/java/org/apache/ignite/raft/jraft/rpc/TestMessageGroup.java ########## @@ -6,32 +6,22 @@ * (the "License"); you may not use this file except in compliance with * the License. You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 + * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.ignite.raft.jraft.rpc.message; -import org.apache.ignite.raft.jraft.rpc.RpcRequests; +package org.apache.ignite.raft.jraft.rpc; -class PingRequestImpl implements RpcRequests.PingRequest, RpcRequests.PingRequest.Builder { - private long sendTimestamp; +import org.apache.ignite.network.annotations.MessageGroup; - @Override public long getSendTimestamp() { - return sendTimestamp; - } - - @Override public Builder setSendTimestamp(long timestamp) { - this.sendTimestamp = timestamp; - - return this; - } - - @Override public RpcRequests.PingRequest build() { - return this; - } +/** + * Message group for tests. + */ +@MessageGroup(groupType = 4, groupName = "TestRaftMessages") Review comment: why? ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/messages/MessageBuilderGenerator.java ########## @@ -73,6 +73,18 @@ public TypeSpec generateBuilderInterface(MessageClass message) { }) .collect(Collectors.toList()); + // generate a getter for each getter in the original interface Review comment: Sorry, can't agree with that, all other comments are in a different style ########## File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/RpcResponseFactory.java ########## @@ -32,45 +33,44 @@ /** * Creates a RPC response from status, return OK response when status is null. * - * @param parent parent message + * @param msgFactory Raft message factory * @param st status with response * @return a response instance */ - default Message newResponse(final Message parent, final Status st) { - if (st == null) { - return newResponse(parent, 0, "OK"); - } - return newResponse(parent, st.getCode(), st.getErrorMsg()); + default Message newResponse(RaftMessagesFactory msgFactory, Status st) { + if (st == null) + return newResponse(msgFactory, 0, "OK"); + + return newResponse(msgFactory, st.getCode(), st.getErrorMsg()); } /** * Creates an error response with parameters. * - * @param parent parent message + * @param msgFactory Raft message factory * @param error error with raft info * @param fmt message with format string * @param args arguments referenced by the format specifiers in the format string * @return a response instance */ - default Message newResponse(final Message parent, final RaftError error, final String fmt, final Object... args) { - return newResponse(parent, error.getNumber(), fmt, args); + default Message newResponse(RaftMessagesFactory msgFactory, RaftError error, String fmt, Object... args) { + return newResponse(msgFactory, error.getNumber(), fmt, args); } /** * Creates an error response with parameters. * - * @param parent parent message + * @param msgFactory Raft message factory * @param code error code with raft info * @param fmt message with format string * @param args arguments referenced by the format specifiers in the format string * @return a response instance */ - default Message newResponse(final Message parent, final int code, final String fmt, final Object... args) { - final RpcRequests.ErrorResponse.Builder eBuilder = RpcRequests.ErrorResponse.newBuilder(); - eBuilder.setErrorCode(code); - if (fmt != null) { - eBuilder.setErrorMsg(String.format(fmt, args)); - } + default Message newResponse(RaftMessagesFactory msgFactory, int code, String fmt, Object... args) { Review comment: also, this "parent" message was always `null`. Maybe it was changed by Alexei, when porting JRaft into our codebase. ########## File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/CliRequests.java ########## @@ -19,443 +19,145 @@ package org.apache.ignite.raft.jraft.rpc; +import java.util.List; +import org.apache.ignite.raft.jraft.RaftMessageGroup; +import org.apache.ignite.network.annotations.Transferable; + public final class CliRequests { + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.ADD_PEER_REQUEST, autoSerializable = false) public interface AddPeerRequest extends Message { - String getGroupId(); - - String getLeaderId(); - - String getPeerId(); - - interface Builder { - Builder setGroupId(String groupId); + String groupId(); - Builder setLeaderId(String leaderId); + String leaderId(); - Builder setPeerId(String peerId); - - AddPeerRequest build(); - } - - public static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createAddPeerRequest(); - } + String peerId(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.ADD_PEER_RESPONSE, autoSerializable = false) public interface AddPeerResponse extends Message { - static Message getDefaultInstance() { - return null; - } - - static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createAddPeerResponse(); - } - - java.util.List<String> getOldPeersList(); - - int getOldPeersCount(); - - String getOldPeers(int index); - - java.util.List<String> getNewPeersList(); - - int getNewPeersCount(); - - String getNewPeers(int index); - - public interface Builder { - Builder addOldPeers(String oldPeersId); - - Builder addNewPeers(String newPeersId); + List<String> oldPeersList(); - AddPeerResponse build(); - } + List<String> newPeersList(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.REMOVE_PEER_REQUEST, autoSerializable = false) public interface RemovePeerRequest extends Message { - String getGroupId(); + String groupId(); - String getLeaderId(); + String leaderId(); - String getPeerId(); - - interface Builder { - Builder setGroupId(String groupId); - - Builder setLeaderId(String leaderId); - - Builder setPeerId(String peerId); - - RemovePeerRequest build(); - } - - public static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createRemovePeerRequest(); - } + String peerId(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.REMOVE_PEER_RESPONSE, autoSerializable = false) public interface RemovePeerResponse extends Message { - static Message getDefaultInstance() { - return null; - } - - static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createRemovePeerResponse(); - } - - java.util.List<String> getOldPeersList(); - - int getOldPeersCount(); + List<String> oldPeersList(); - String getOldPeers(int index); - - java.util.List<String> getNewPeersList(); - - int getNewPeersCount(); - - String getNewPeers(int index); - - public interface Builder { - Builder addOldPeers(String oldPeerId); - - Builder addNewPeers(String newPeerId); - - RemovePeerResponse build(); - } + List<String> newPeersList(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.CHANGE_PEERS_REQUEST, autoSerializable = false) public interface ChangePeersRequest extends Message { - String getGroupId(); - - String getLeaderId(); + String groupId(); - java.util.List<String> getNewPeersList(); + String leaderId(); - int getNewPeersCount(); - - String getNewPeers(int index); - - public interface Builder { - Builder setGroupId(String groupId); - - Builder setLeaderId(String leaderId); - - Builder addNewPeers(String peerId); - - ChangePeersRequest build(); - } - - public static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createChangePeerRequest(); - } + List<String> newPeersList(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.CHANGE_PEERS_RESPONSE, autoSerializable = false) public interface ChangePeersResponse extends Message { + List<String> oldPeersList(); - static Message getDefaultInstance() { - return null; - } - - static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createChangePeerResponse(); - } - - java.util.List<String> getOldPeersList(); - - int getOldPeersCount(); - - String getOldPeers(int index); - - java.util.List<String> getNewPeersList(); - - int getNewPeersCount(); - - String getNewPeers(int index); - - public interface Builder { - Builder addOldPeers(String oldPeerId); - - Builder addNewPeers(String newPeerId); - - ChangePeersResponse build(); - } + List<String> newPeersList(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.SNAPSHOT_REQUEST, autoSerializable = false) public interface SnapshotRequest extends Message { - String getGroupId(); - - String getPeerId(); - - public interface Builder { - Builder setGroupId(String groupId); - - Builder setPeerId(String peerId); + String groupId(); - SnapshotRequest build(); - } - - static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createSnapshotRequest(); - } + String peerId(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.RESET_PEER_REQUEST, autoSerializable = false) public interface ResetPeerRequest extends Message { - String getGroupId(); - - String getPeerId(); - - java.util.List<String> getOldPeersList(); - - int getOldPeersCount(); - - String getOldPeers(int index); - - java.util.List<String> getNewPeersList(); - - int getNewPeersCount(); - - String getNewPeers(int index); - - public interface Builder { - Builder setGroupId(String groupId); - - Builder setPeerId(String peerId); + String groupId(); - Builder addNewPeers(String peerId); + String peerId(); - ResetPeerRequest build(); - } + List<String> oldPeersList(); - public static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createResetPeerRequest(); - } + List<String> newPeersList(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.TRANSFER_LEADER_REQUEST, autoSerializable = false) public interface TransferLeaderRequest extends Message { - String getGroupId(); + String groupId(); - String getLeaderId(); + String leaderId(); - String getPeerId(); - - boolean hasPeerId(); - - public interface Builder { - Builder setGroupId(String groupId); - - Builder setLeaderId(String leaderId); - - Builder setPeerId(String peerId); - - TransferLeaderRequest build(); - } - - public static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createTransferLeaderRequest(); - } + String peerId(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.GET_LEADER_REQUEST, autoSerializable = false) public interface GetLeaderRequest extends Message { - String getGroupId(); - - String getPeerId(); - - boolean hasPeerId(); + String groupId(); - public interface Builder { - Builder setGroupId(String groupId); - - Builder setPeerId(String peerId); - - GetLeaderRequest build(); - } - - public static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createGetLeaderRequest(); - } + String peerId(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.GET_LEADER_RESPONSE, autoSerializable = false) public interface GetLeaderResponse extends Message { - static Message getDefaultInstance() { - return null; - } - - static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createGetLeaderResponse(); - } - - String getLeaderId(); - - public interface Builder { - GetLeaderResponse build(); - - Builder setLeaderId(String leaderId); - } + String leaderId(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.GET_PEERS_REQUEST, autoSerializable = false) public interface GetPeersRequest extends Message { - String getGroupId(); + String groupId(); - String getLeaderId(); + String leaderId(); - boolean getOnlyAlive(); - - public interface Builder { - Builder setGroupId(String groupId); - - Builder setLeaderId(String leaderId); - - Builder setOnlyAlive(boolean onlyGetAlive); - - GetPeersRequest build(); - } - - public static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createGetPeersRequest(); - } + boolean onlyAlive(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.GET_PEERS_RESPONSE, autoSerializable = false) public interface GetPeersResponse extends Message { - static Message getDefaultInstance() { - return null; - } - - static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createGetPeersResponse(); - } - - java.util.List<String> getPeersList(); - - int getPeersCount(); + List<String> peersList(); - String getPeers(int index); - - java.util.List<String> getLearnersList(); - - int getLearnersCount(); - - String getLearners(int index); - - public interface Builder { - Builder addPeers(String peerId); - - Builder addLearners(String learnerId); - - GetPeersResponse build(); - } + List<String> learnersList(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.ADD_LEARNERS_REQUEST, autoSerializable = false) public interface AddLearnersRequest extends Message { - String getGroupId(); - - String getLeaderId(); + String groupId(); - java.util.List<String> getLearnersList(); + String leaderId(); - int getLearnersCount(); - - String getLearners(int index); - - public interface Builder { - Builder setGroupId(String groupId); - - Builder setLeaderId(String leaderId); - - Builder addLearners(String learnerId); - - AddLearnersRequest build(); - } - - static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createAddLearnersRequest(); - } + List<String> learnersList(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.REMOVE_LEARNERS_REQUEST, autoSerializable = false) public interface RemoveLearnersRequest extends Message { - String getGroupId(); - - String getLeaderId(); - - java.util.List<String> getLearnersList(); + String groupId(); - int getLearnersCount(); + String leaderId(); - String getLearners(int index); - - public interface Builder { - Builder setGroupId(String groupId); - - Builder setLeaderId(String leaderId); - - Builder addLearners(String leaderId); - - RemoveLearnersRequest build(); - } - - static Builder newBuilder() { - return MessageBuilderFactory.DEFAULT.createRemoveLearnersRequest(); - } + List<String> learnersList(); } + @Transferable(value = RaftMessageGroup.RpcClientMessageGroup.RESET_LEARNERS_REQUEST, autoSerializable = false) public interface ResetLearnersRequest extends Message { - String getGroupId(); - - String getLeaderId(); - - java.util.List<String> getLearnersList(); + String groupId(); - /** - * <code>repeated string learners = 3;</code> - */ - int getLearnersCount(); + String leaderId(); - /** - * <code>repeated string learners = 3;</code> Review comment: no idea ########## File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/FSMCallerImpl.java ########## @@ -647,18 +649,20 @@ private void doSnapshotLoad(final LoadSnapshotClosure done) { setError(e); return; } - if (meta.getOldPeersCount() == 0) { + if (meta.oldPeersList() == null) { // Joint stage is not supposed to be noticeable by end users. final Configuration conf = new Configuration(); - for (int i = 0, size = meta.getPeersCount(); i < size; i++) { - final PeerId peer = new PeerId(); - Requires.requireTrue(peer.parse(meta.getPeers(i)), "Parse peer failed"); - conf.addPeer(peer); + if (meta.peersList() != null) { Review comment: yes, it can ########## File path: modules/raft/src/test/java/org/apache/ignite/raft/jraft/rpc/TestMessageGroup.java ########## @@ -6,32 +6,22 @@ * (the "License"); you may not use this file except in compliance with * the License. You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 + * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.ignite.raft.jraft.rpc.message; -import org.apache.ignite.raft.jraft.rpc.RpcRequests; +package org.apache.ignite.raft.jraft.rpc; -class PingRequestImpl implements RpcRequests.PingRequest, RpcRequests.PingRequest.Builder { - private long sendTimestamp; +import org.apache.ignite.network.annotations.MessageGroup; - @Override public long getSendTimestamp() { - return sendTimestamp; - } - - @Override public Builder setSendTimestamp(long timestamp) { - this.sendTimestamp = timestamp; - - return this; - } - - @Override public RpcRequests.PingRequest build() { - return this; - } +/** + * Message group for tests. + */ +@MessageGroup(groupType = 4, groupName = "TestRaftMessages") Review comment: where? I can only see a problem if you start using test messages from one module in another, but I don't think this is a viable use case. ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/messages/MessageBuilderGenerator.java ########## @@ -73,6 +73,18 @@ public TypeSpec generateBuilderInterface(MessageClass message) { }) .collect(Collectors.toList()); + // generate a getter for each getter in the original interface Review comment: we had a convention (which is not documented, AFAIK), but not for simple commentaries ########## File path: modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/messages/MessageBuilderGenerator.java ########## @@ -73,6 +73,18 @@ public TypeSpec generateBuilderInterface(MessageClass message) { }) .collect(Collectors.toList()); + // generate a getter for each getter in the original interface Review comment: we had a convention for javadocs (which is not documented, AFAIK), but not for simple commentaries -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
