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/30 03:23:22 UTC
[GitHub] [incubator-ratis] runzhiwang opened a new pull request #385: RATIS-1207. Fix duplicated StreamMap#Key
runzhiwang opened a new pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385
## What changes were proposed in this pull request?
Fix duplicated StreamMap#Key
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1207
## How was this patch tested?
new assert
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385#discussion_r550041368
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -170,10 +170,12 @@ public String toString() {
static class StreamMap {
static class Key {
private final ChannelId channelId;
+ private final ClientId clientId;
private final long streamId;
- Key(ChannelId channelId, long streamId) {
+ Key(ChannelId channelId, ClientId clientId, long streamId) {
Review comment:
Could we remove channelId since we have clientId? If yes, we can use ClientInvocationId and remove the entire StreamMap.Key class.
##########
File path: ratis-proto/src/main/proto/Raft.proto
##########
@@ -327,6 +327,7 @@ message DataStreamPacketHeaderProto {
Type type = 3;
repeated Option options = 4;
uint64 dataLength = 5;
+ bytes clientId = 6;
Review comment:
Since Streaming is not yet released, how about we take this chance to renumber the proto?
```
bytes clientId = 1;
Type type = 2;
uint64 streamId = 3;
uint64 streamOffset = 4;
uint64 dataLength = 5;
repeated Option options = 6;
```
Then, the proto and the java class will have the same order
```
public DataStreamRequestHeader(ClientId clientId, Type type, long streamId, long streamOffset, long dataLength,
WriteOption... options) {
```
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385#issuecomment-752381531
@szetszwo Thanks for review. I will work on the suggestions.
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385#issuecomment-752381079
Since this already has passed all the checks, let's merge this first and work on the suggestions separately.
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385#discussion_r550056126
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -170,10 +170,12 @@ public String toString() {
static class StreamMap {
static class Key {
private final ChannelId channelId;
+ private final ClientId clientId;
private final long streamId;
- Key(ChannelId channelId, long streamId) {
+ Key(ChannelId channelId, ClientId clientId, long streamId) {
Review comment:
Yes, we can remove channelId
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385#issuecomment-752372362
ready for review
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385#issuecomment-752342620
Sorry, still need some change
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385
----------------------------------------------------------------
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 #385: RATIS-1207. Fix duplicated StreamMap#Key
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #385:
URL: https://github.com/apache/incubator-ratis/pull/385
----------------------------------------------------------------
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