You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "waitinfuture (via GitHub)" <gi...@apache.org> on 2023/02/13 04:49:20 UTC
[GitHub] [incubator-celeborn] waitinfuture commented on a diff in pull request #1214: [CELEBORN-278] Refactor open stream to support buffer stream.
waitinfuture commented on code in PR #1214:
URL: https://github.com/apache/incubator-celeborn/pull/1214#discussion_r1103981942
##########
common/src/main/java/org/apache/celeborn/common/network/protocol/OpenStream.java:
##########
@@ -24,25 +24,13 @@
import io.netty.buffer.ByteBuf;
/** Request to read a set of blocks. Returns {@link StreamHandle}. */
-public final class OpenStream extends RequestMessage {
+public class OpenStream extends RequestMessage {
public byte[] shuffleKey;
public byte[] fileName;
- public int startMapIndex;
- public int endMapIndex;
- public OpenStream(String shuffleKey, String fileName, int startMapIndex, int endMapIndex) {
- this(
- shuffleKey.getBytes(StandardCharsets.UTF_8),
- fileName.getBytes(StandardCharsets.UTF_8),
- startMapIndex,
- endMapIndex);
- }
-
- public OpenStream(byte[] shuffleKey, byte[] fileName, int startMapIndex, int endMapIndex) {
+ public OpenStream(byte[] shuffleKey, byte[] fileName) {
Review Comment:
```encodedLength``` should also be changed as startMapIndex and endMapIndex are removed
##########
common/src/main/java/org/apache/celeborn/common/network/protocol/OpenStream.java:
##########
@@ -24,25 +24,13 @@
import io.netty.buffer.ByteBuf;
/** Request to read a set of blocks. Returns {@link StreamHandle}. */
-public final class OpenStream extends RequestMessage {
+public class OpenStream extends RequestMessage {
public byte[] shuffleKey;
public byte[] fileName;
- public int startMapIndex;
- public int endMapIndex;
- public OpenStream(String shuffleKey, String fileName, int startMapIndex, int endMapIndex) {
- this(
- shuffleKey.getBytes(StandardCharsets.UTF_8),
- fileName.getBytes(StandardCharsets.UTF_8),
- startMapIndex,
- endMapIndex);
- }
-
- public OpenStream(byte[] shuffleKey, byte[] fileName, int startMapIndex, int endMapIndex) {
+ public OpenStream(byte[] shuffleKey, byte[] fileName) {
Review Comment:
I think refactoring OpenStream will cause incompatibility issue. I think we can try renaming OpenStream to OpenChunkStream, and we should run test case with newer client and older worker to verify.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org