You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/12/19 01:06:18 UTC
[GitHub] [incubator-ratis] runzhiwang opened a new pull request #364: RATIS-1248. Support network topology
runzhiwang opened a new pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364
## What changes were proposed in this pull request?
Not only support primary -> peer1 && peer2, but also support primary -> peer1 -> peer2.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1248
## How was this patch tested?
TODO
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546216876
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -231,23 +232,48 @@ StreamInfo remove(Key key) {
return f;
}
+
+ private IdentityHashMap<RaftPeer, RaftPeer> getRouteTable(RaftClientRequest request)
Review comment:
IdentityHashMap may have unexpected behaviour since it uses == but not equals(..). Let's use
```
Map<RaftPeerId, List<RaftPeerId>>
```
Or, we may use guava Multimap https://guava.dev/releases/19.0/api/docs/com/google/common/collect/Multimap.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546192544
##########
File path: ratis-proto/src/main/proto/Raft.proto
##########
@@ -280,6 +280,19 @@ message WatchRequestTypeProto {
ReplicationLevel replication = 2;
}
+message RoutePairProto {
+ RaftPeerProto from = 1;
+ RaftPeerProto to = 2;
+}
+
+message DataStreamRouteTableProto {
+ repeated RoutePairProto routeTable = 1;
+}
Review comment:
Let's call them RouteProto and RoutingTableProto. It may be useful outside DataStream in the future.
Let's use RaftPeerId instead of RaftPeerProto. It may be a good time to create RaftPeerIdProto which is currently missing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546194092
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -231,23 +232,48 @@ StreamInfo remove(Key key) {
return f;
}
+
+ private IdentityHashMap<RaftPeer, RaftPeer> getRouteTable(RaftClientRequest request)
Review comment:
Let's create a org.apache.ratis.protocol.RouteTable interface/class in ratis-common.
```
interface RoutingTable {
List<RaftPeerId> getSuccessors(RaftPeerId peer);
}
```
I think we need RaftPeerId since the RaftPeer can be looked up locally.
BTW, why using IdentityHashMap but not just a Map in the implementation?
##########
File path: ratis-proto/src/main/proto/Raft.proto
##########
@@ -280,6 +280,19 @@ message WatchRequestTypeProto {
ReplicationLevel replication = 2;
}
+message RoutePairProto {
+ RaftPeerProto from = 1;
+ RaftPeerProto to = 2;
+}
+
+message DataStreamRouteTableProto {
+ repeated RoutePairProto routeTable = 1;
+}
Review comment:
Let's call the RouteProto and RouteTableProto. It may be useful outside DataStream in the future.
Let's use RaftPeerId instead of RaftPeerProto. It may be a good time to create RaftPeerIdProto which is currently missing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546495447
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
@szetszwo Thanks, really learn a lot.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#issuecomment-748569007
@szetszwo I have updated the patch, could you help review it again ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546351023
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
Let's add a Builder.
```
public interface RoutingTable {
Set<RaftPeerId> getSuccessors(RaftPeerId peerId);
class Builder {
private final Map<RaftPeerId, Set<RaftPeerId>> map = new HashMap<>();
private Set<RaftPeerId> computeIfAbsent(RaftPeerId peerId) {
return map.computeIfAbsent(peerId, key -> new HashSet<>());
}
public Builder addSuccessor(RaftPeerId peerId, RaftPeerId successor) {
computeIfAbsent(peerId).add(successor);
return this;
}
public Builder addSuccessors(RaftPeerId peerId, List<RaftPeerId> successors) {
computeIfAbsent(peerId).addAll(successors);
return this;
}
public Builder addSuccessors(RaftPeerId peerId, RaftPeerId... successors) {
return addSuccessors(peerId, Arrays.asList(successors));
}
public RoutingTable build() {
return peerId -> Optional.ofNullable(map.get(peerId)).orElseGet(Collections::emptySet);
}
}
}
```
In the code, we should keeping using RoutingTable instead of Map. It will give us more flexibility.
Btw, it should return a Set instead of List. My bad!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang edited a comment on pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#issuecomment-748396823
@szetszwo Hi, sorry, now I have one problem, where should I put the route table in proto ? Now I put route table in DataStreamInitProto, and encode DataStreamInitProto as RaftClientRequestProto#message. But this can not work for filestore, because filestore encode FileStoreRequestProto as RaftClientRequestProto#message, I can only put route table in StreamWriteRequestProto. But this seems a little complicated, maybe we should put route table in a same proto, no matter whether it's filestore. Do you have any suggestion ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#issuecomment-748587147
RoutingTable is a small interface to encapsulate the getSuccessors(..) method. We want to use it instead of Map since we don't have to build a Map in some cases. E.g. the default uses a RaftConfigurable to return the other peer ids.
BTW, StreamInfo does not have to implement RoutingTable. It may just provide a getSuccessors method as below.
```
// StreamInfo
private Set<RaftPeer> getSuccessors(RaftPeerId peerId, RaftConfiguration conf) {
final RoutingTable routingTable = ...;
if (routingTable != null) {
return routingTable.getSuccessors(peerId).stream().map(conf::getPeer).collect(Collectors.toSet());
} if (isPrimary()) {
return conf.getCurrentPeers().stream()
.filter(p -> !p.getId().equals(server.getId()))
.collect(Collectors.toSet());
} else {
return Collections.emptySet();
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang closed pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546465118
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
That is a good point. We should add a toProto() method to the interface.
```
public interface RoutingTable {
Set<RaftPeerId> getSuccessors(RaftPeerId peerId);
RoutingTableProto toProto();
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang closed pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546331656
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
It should use RoutingTable as the type.
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -145,6 +150,22 @@ LocalStream getLocal() {
public String toString() {
return JavaUtils.getClassSimpleName(getClass()) + ":" + request;
}
+
+ @Override
+ public List<RaftPeerId> getSuccessors(RaftPeerId peerId) throws IOException {
+ Map<RaftPeerId, List<RaftPeerId>> routingTable = request.getRoutingTable();
+ if (routingTable.containsKey(peerId)) {
+ return routingTable.get(peerId);
+ } else if (isPrimary()) {
+ // TODO(runzhiwang): if filestore support route table, we can remove this branch
Review comment:
We should keep this branch as the default for the applications which does not support RoutingTable.
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -145,6 +150,22 @@ LocalStream getLocal() {
public String toString() {
return JavaUtils.getClassSimpleName(getClass()) + ":" + request;
}
+
+ @Override
+ public List<RaftPeerId> getSuccessors(RaftPeerId peerId) throws IOException {
+ Map<RaftPeerId, List<RaftPeerId>> routingTable = request.getRoutingTable();
+ if (routingTable.containsKey(peerId)) {
+ return routingTable.get(peerId);
+ } else if (isPrimary()) {
+ // TODO(runzhiwang): if filestore support route table, we can remove this branch
+ final RaftGroupId groupId = request.getRaftGroupId();
+ return server.getDivision(groupId).getRaftConf().getCurrentPeers().stream()
+ .filter(p -> !p.getId().equals(server.getId())).map(RaftPeer::getId)
+ .collect(Collectors.toList());
+ } else {
+ return new ArrayList<>();
Review comment:
Use `Collections.emptyList()`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546455511
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
@szetszwo Maybe we also need to expose the map to user, because we need encode map to proto, the lambda `peerId -> Optional.ofNullable(map.get(peerId)).orElseGet(Collections::emptySet)` is not enough.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo merged pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546485981
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
The build() method needs to be updated. We should also change the Builder to use a reference so that it cannot be built more than once.
```
public interface RoutingTable {
Set<RaftPeerId> getSuccessors(RaftPeerId peerId);
RoutingTableProto toProto();
class Builder {
private final AtomicReference<Map<RaftPeerId, Set<RaftPeerId>>> ref = new AtomicReference<>(new HashMap<>());
private Set<RaftPeerId> computeIfAbsent(RaftPeerId peerId) {
return Optional.ofNullable(ref.get())
.map(map -> map.computeIfAbsent(peerId, key -> new HashSet<>()))
.orElseThrow(() -> new IllegalStateException("Already built"));
}
public Builder addSuccessor(RaftPeerId peerId, RaftPeerId successor) {
computeIfAbsent(peerId).add(successor);
return this;
}
public Builder addSuccessors(RaftPeerId peerId, Collection<RaftPeerId> successors) {
computeIfAbsent(peerId).addAll(successors);
return this;
}
public Builder addSuccessors(RaftPeerId peerId, RaftPeerId... successors) {
return addSuccessors(peerId, Arrays.asList(successors));
}
public RoutingTable build() {
return Optional.ofNullable(ref.getAndSet(null))
.map(RoutingTable::newRoutingTable)
.orElseThrow(() -> new IllegalStateException("Already built"));
}
}
static RoutingTable newRoutingTable(Map<RaftPeerId, Set<RaftPeerId>> map){
return new RoutingTable() {
@Override
public Set<RaftPeerId> getSuccessors(RaftPeerId peerId) {
return Optional.ofNullable(map.get(peerId)).orElseGet(Collections::emptySet);
}
@Override
public RoutingTableProto toProto() {
return RoutingTableProto.newBuilder().addAllRoutingTable(ProtoUtils.toRouteProtos(map)).build();
}
};
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang closed pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang closed pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546217212
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -231,23 +232,48 @@ StreamInfo remove(Key key) {
return f;
}
+
+ private IdentityHashMap<RaftPeer, RaftPeer> getRouteTable(RaftClientRequest request)
Review comment:
Got it, I will use `Map<RaftPeerId, List<RaftPeerId>>` Thanks a lot.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546213601
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -231,23 +232,48 @@ StreamInfo remove(Key key) {
return f;
}
+
+ private IdentityHashMap<RaftPeer, RaftPeer> getRouteTable(RaftClientRequest request)
Review comment:
IdentityHashMap allows duplicated key, such as primary -> peer1, primary -> peer2, we can put them in IdentityHashMap, but can not put them in a map which do not allow duplicated key.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546336403
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
@szetszwo RoutingTable has been defined as a interface as follows, maybe we need to defined another class ?
```
interface RoutingTable {
List<RaftPeerId> getSuccessors(RaftPeerId peer);
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546455511
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
@szetszwo Maybe we also need to expose the map to user, because we need to encode RoutingTable in RaftClientRequest to proto, the lambda `peerId -> Optional.ofNullable(map.get(peerId)).orElseGet(Collections::emptySet)` is not enough.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546506442
##########
File path: ratis-proto/src/main/proto/Raft.proto
##########
@@ -280,6 +284,15 @@ message WatchRequestTypeProto {
ReplicationLevel replication = 2;
}
+message RouteProto {
+ RaftPeerIdProto from = 1;
+ repeated RaftPeerIdProto to = 2;
+}
+
+message RoutingTableProto {
+ repeated RouteProto routingTable = 1;
Review comment:
Rename this to `routes`.
##########
File path: ratis-proto/src/main/proto/Raft.proto
##########
@@ -280,6 +284,15 @@ message WatchRequestTypeProto {
ReplicationLevel replication = 2;
}
+message RouteProto {
+ RaftPeerIdProto from = 1;
+ repeated RaftPeerIdProto to = 2;
Review comment:
To be consistent with RoutingTable.java, let's rename the fields to peerId and successors.
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -145,6 +148,26 @@ LocalStream getLocal() {
public String toString() {
return JavaUtils.getClassSimpleName(getClass()) + ":" + request;
}
+
+ private Set<RaftPeer> getSuccessors(RaftPeerId peerId) throws IOException {
+ final RoutingTable routingTable = request.getRoutingTable();
+ final RaftGroupId groupId = request.getRaftGroupId();
+ final RaftConfiguration conf = server.getDivision(groupId).getRaftConf();
+ Set<RaftPeer> successors = routingTable.getSuccessors(peerId).stream().map(conf::getPeer)
+ .collect(Collectors.toSet());
+ if (successors.size() > 0) {
Review comment:
If successors.size() == 0, either there is no routingTable, or there is no successors in the route. We need to handle this two cases separately.
When there is no routing table, we should change request.getRoutingTable() to return null.
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/ClientProtoUtils.java
##########
@@ -118,14 +118,23 @@
static RaftClientRequest toRaftClientRequest(RaftClientRequestProto p) {
final RaftClientRequest.Type type = toRaftClientRequestType(p);
final RaftRpcRequestProto request = p.getRpcRequest();
+ RoutingTable.Builder routingTablebuilder = new RoutingTable.Builder();
Review comment:
Check `p.hasRoutingTable()`. Pass null if `p.hasRoutingTable() == false`.
##########
File path: ratis-common/src/main/java/org/apache/ratis/protocol/RaftClientRequest.java
##########
@@ -235,17 +235,28 @@ public static RaftClientRequest toWriteRequest(RaftClientRequest r, Message mess
private final SlidingWindowEntry slidingWindowEntry;
+ private final RoutingTable routingTable;
+
public RaftClientRequest(ClientId clientId, RaftPeerId serverId, RaftGroupId groupId, long callId, Type type) {
- this(clientId, serverId, groupId, callId, null, type, null);
+ this(clientId, serverId, groupId, callId, null, type, null, null);
}
public RaftClientRequest(
ClientId clientId, RaftPeerId serverId, RaftGroupId groupId,
long callId, Message message, Type type, SlidingWindowEntry slidingWindowEntry) {
+ this(clientId, serverId, groupId, callId, message, type, slidingWindowEntry, null);
+ }
+
+ @SuppressWarnings("parameternumber")
+ public RaftClientRequest(
+ ClientId clientId, RaftPeerId serverId, RaftGroupId groupId,
+ long callId, Message message, Type type, SlidingWindowEntry slidingWindowEntry,
+ RoutingTable routingTable) {
super(clientId, serverId, groupId, callId);
this.message = message;
this.type = type;
this.slidingWindowEntry = slidingWindowEntry != null? slidingWindowEntry: SlidingWindowEntry.getDefaultInstance();
+ this.routingTable = routingTable != null ? routingTable : new RoutingTable.Builder().build();
Review comment:
Just set this.routingTable to null if there is no routingTable.
##########
File path: ratis-common/src/main/java/org/apache/ratis/protocol/RoutingTable.java
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (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
+ *
+ * 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.ratis.protocol;
+
+import org.apache.ratis.proto.RaftProtos.RoutingTableProto;
+import org.apache.ratis.util.ProtoUtils;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
+
+public interface RoutingTable {
+ Set<RaftPeerId> getSuccessors(RaftPeerId peerId);
+
+ RoutingTableProto toProto();
+
+ class Builder {
Review comment:
I forgot to add a `newBuilder()` method and a private constructor.
```
private Builder() {}
```
Please add them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546217212
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -231,23 +232,48 @@ StreamInfo remove(Key key) {
return f;
}
+
+ private IdentityHashMap<RaftPeer, RaftPeer> getRouteTable(RaftClientRequest request)
Review comment:
Got it, I will use Map<RaftPeerId, List<RaftPeerId>>. Thanks a lot.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#issuecomment-748445410
> We probably should add the RoutingTableProto to RaftClientRequestProto
@szetszwo I agree.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546194092
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -231,23 +232,48 @@ StreamInfo remove(Key key) {
return f;
}
+
+ private IdentityHashMap<RaftPeer, RaftPeer> getRouteTable(RaftClientRequest request)
Review comment:
Let's create a org.apache.ratis.protocol.RoutingTable interface/class in ratis-common.
```
interface RoutingTable {
List<RaftPeerId> getSuccessors(RaftPeerId peer);
}
```
I think we should use RaftPeerId since the RaftPeer can be looked up locally.
BTW, why using IdentityHashMap but not just a Map in the implementation?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang edited a comment on pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#issuecomment-748396823
@szetszwo Hi, sorry, now I have one problem, where should I put the route table in proto ? Now I put route table in DataStreamInitProto, and encode DataStreamInitProto as RaftClientRequestProto#message. But this can not work for filestore, because filestore encode FileStoreRequestProto as RaftClientRequestProto#message, I can only put route table in StreamWriteRequestProto. But this seems a little complicated, maybe we should put route table in a same proto, no matter whether it's filestore, to achieve this, maybe we should put Raft.proto and Examples.proto together. Do you have any suggestion ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546336403
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
@szetszwo RoutingTable has been defined as an interface as follows, maybe we need to defined another class ?
```
interface RoutingTable {
List<RaftPeerId> getSuccessors(RaftPeerId peer);
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#issuecomment-748422745
BTW, let's separate the FileStore change to another JIRA since RoutingTable is an important change. We should focus on it.
Thanks a lot for working on this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #364: RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#discussion_r546467443
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/DataStreamApi.java
##########
@@ -44,4 +48,7 @@ default DataStreamOutput stream() {
/** Create a stream by providing a customized header message. */
DataStreamOutput stream(ByteBuffer headerMessage);
+
+ /** Create a stream by providing a customized header message and route table. */
+ DataStreamOutput stream(ByteBuffer headerMessage, Map<RaftPeerId, List<RaftPeerId>> routingTable);
Review comment:
@szetszwo Sorry, not familar with Java, but the following code compiled error.
```
public interface RoutingTable {
Set<RaftPeerId> getSuccessors(RaftPeerId peerId);
RoutingTableProto toProto();
class Builder {
private final Map<RaftPeerId, Set<RaftPeerId>> map = new HashMap<>();
private Set<RaftPeerId> computeIfAbsent(RaftPeerId peerId) {
return map.computeIfAbsent(peerId, key -> new HashSet<>());
}
public Builder addSuccessor(RaftPeerId peerId, RaftPeerId successor) {
computeIfAbsent(peerId).add(successor);
return this;
}
public Builder addSuccessors(RaftPeerId peerId, List<RaftPeerId> successors) {
computeIfAbsent(peerId).addAll(successors);
return this;
}
public Builder addSuccessors(RaftPeerId peerId, RaftPeerId... successors) {
return addSuccessors(peerId, Arrays.asList(successors));
}
public RoutingTable build() {
return peerId -> Optional.ofNullable(map.get(peerId)).orElseGet(Collections::emptySet);
}
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on pull request #364: [WIP]RATIS-1248. Support network topology
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #364:
URL: https://github.com/apache/incubator-ratis/pull/364#issuecomment-748396823
@szetszwo Hi, sorry, now I have one problem, where should I put the route table in proto ? Now I put route table in DataStreamInitProto, and encode DataStreamInitProto as RaftClientRequestProto#message. But this can not work for filestore, because filestore encode FileStoreRequestProto as RaftClientRequestProto#message, I can only put route table in StreamWriteRequestProto. But this seems a little complicated, maybe we should put route table in a same proto, no matter whether it's filestore. Do you have any suggestion ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org