You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celeborn.apache.org by GitBox <gi...@apache.org> on 2022/11/23 09:26:23 UTC
[GitHub] [incubator-celeborn] FMX opened a new pull request, #1000: [CELEBORN-50] Fix fetch chunk illegal state exception.
FMX opened a new pull request, #1000:
URL: https://github.com/apache/incubator-celeborn/pull/1000
# [CELEBORN-50][BUG] Fix fetch chunk illegal state exception.
### What changes were proposed in this pull request?
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### What are the items that need reviewer attention?
### Related issues.
### Related pull requests.
### How was this patch tested?
/cc @related-reviewer
/assign @main-reviewer
--
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: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] FMX merged pull request #1000: [CELEBORN-50] Channel inActive may cause new client use old stream id to fetch data cause IllegalStateException.
Posted by GitBox <gi...@apache.org>.
FMX merged PR #1000:
URL: https://github.com/apache/incubator-celeborn/pull/1000
--
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: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] FMX commented on a diff in pull request #1000: [CELEBORN-50] Fix fetch chunk illegal state exception.
Posted by GitBox <gi...@apache.org>.
FMX commented on code in PR #1000:
URL: https://github.com/apache/incubator-celeborn/pull/1000#discussion_r1030217664
##########
client/src/main/java/org/apache/celeborn/client/read/Replica.java:
##########
@@ -58,7 +58,12 @@ class Replica {
public synchronized TransportClient getOrOpenStream() throws IOException, InterruptedException {
if (client == null || !client.isActive()) {
client = clientFactory.createClient(location.getHost(), location.getFetchPort());
+ OpenStream openBlocks =
+ new OpenStream(shuffleKey, location.getFileName(), startMapIndex, endMapIndex);
+ ByteBuffer response = client.sendRpcSync(openBlocks.toByteBuffer(), timeoutMs);
+ streamHandle = (StreamHandle) Message.decode(response);
Review Comment:
It is updated.
--
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: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] FMX commented on a diff in pull request #1000: [CELEBORN-50] Channel inActive may cause new client use old stream id to fetch data cause IllegalStateException.
Posted by GitBox <gi...@apache.org>.
FMX commented on code in PR #1000:
URL: https://github.com/apache/incubator-celeborn/pull/1000#discussion_r1030227671
##########
client/src/main/java/org/apache/celeborn/client/read/Replica.java:
##########
@@ -58,16 +58,22 @@ class Replica {
public synchronized TransportClient getOrOpenStream() throws IOException, InterruptedException {
if (client == null || !client.isActive()) {
Review Comment:
How about now ?
--
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: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1000: [CELEBORN-50] Fix fetch chunk illegal state exception.
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1000:
URL: https://github.com/apache/incubator-celeborn/pull/1000#discussion_r1030220245
##########
client/src/main/java/org/apache/celeborn/client/read/Replica.java:
##########
@@ -58,16 +58,22 @@ class Replica {
public synchronized TransportClient getOrOpenStream() throws IOException, InterruptedException {
if (client == null || !client.isActive()) {
Review Comment:
```
if (client != null) {
logger.error(
"Current channel from "
+ client.getChannel().localAddress()
+ " to "
+ client.getChannel().remoteAddress()
+ " for "
+ this
+ " is not active.");
}
```
--
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: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1000: [CELEBORN-50] Fix fetch chunk illegal state exception.
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1000:
URL: https://github.com/apache/incubator-celeborn/pull/1000#discussion_r1030218347
##########
client/src/main/java/org/apache/celeborn/client/read/Replica.java:
##########
@@ -58,16 +58,22 @@ class Replica {
public synchronized TransportClient getOrOpenStream() throws IOException, InterruptedException {
if (client == null || !client.isActive()) {
Review Comment:
log for client is not active?
--
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: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] AngersZhuuuu commented on pull request #1000: [CELEBORN-50] Fix fetch chunk illegal state exception.
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #1000:
URL: https://github.com/apache/incubator-celeborn/pull/1000#issuecomment-1324784685
pr totle
```
Channel inActive may cause new client use old stream id to fetch data cause IllegalStateException
```
--
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: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1000: [CELEBORN-50] Fix fetch chunk illegal state exception.
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1000:
URL: https://github.com/apache/incubator-celeborn/pull/1000#discussion_r1030212204
##########
client/src/main/java/org/apache/celeborn/client/read/Replica.java:
##########
@@ -58,7 +58,12 @@ class Replica {
public synchronized TransportClient getOrOpenStream() throws IOException, InterruptedException {
if (client == null || !client.isActive()) {
client = clientFactory.createClient(location.getHost(), location.getFetchPort());
+ OpenStream openBlocks =
+ new OpenStream(shuffleKey, location.getFileName(), startMapIndex, endMapIndex);
+ ByteBuffer response = client.sendRpcSync(openBlocks.toByteBuffer(), timeoutMs);
+ streamHandle = (StreamHandle) Message.decode(response);
Review Comment:
Just `streamHandle == null` ?
Then same code won't write twice.
Also for client.isActive == false, we can have a log to indicate this case
--
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: dev-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org