[ISSUE #4701]Fix use tcp protocol client send message, it throw a DecoderException #4702
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,7 @@ | |
| import lombok.extern.slf4j.Slf4j; | ||
|
|
||
| @Slf4j | ||
| public class Codec { | ||
|
Check warning on line 45 in eventmesh-common/src/main/java/org/apache/eventmesh/common/protocol/tcp/codec/Codec.java
|
||
|
|
||
| private static final int FRAME_MAX_LENGTH = 1024 * 1024 * 4; | ||
|
|
||
|
|
@@ -104,7 +104,7 @@ | |
| public static class Decoder extends LengthFieldBasedFrameDecoder { | ||
|
|
||
| public Decoder() { | ||
| super(FRAME_MAX_LENGTH, PREFIX_LENGTH, Integer.BYTES, -9, 0); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -115,7 +115,7 @@ | |
| try { | ||
| target = (ByteBuf) super.decode(ctx, in); | ||
| if (null == target) { | ||
| return null; | ||
|
Check warning on line 118 in eventmesh-common/src/main/java/org/apache/eventmesh/common/protocol/tcp/codec/Codec.java
|
||
| } | ||
| byte[] flagBytes = parseFlag(target); | ||
| byte[] versionBytes = parseVersion(target); | ||
|
|
@@ -130,16 +130,16 @@ | |
| Package pkg = new Package(header, body); | ||
| return pkg; | ||
|
|
||
| } catch (Exception ex) { | ||
| log.error("decode error", ex); | ||
| ctx.channel().close(); | ||
|
Check warning on line 135 in eventmesh-common/src/main/java/org/apache/eventmesh/common/protocol/tcp/codec/Codec.java
|
||
| } finally { | ||
| if (target != null) { | ||
| target.release(); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
|
Check warning on line 142 in eventmesh-common/src/main/java/org/apache/eventmesh/common/protocol/tcp/codec/Codec.java
|
||
| } | ||
|
|
||
| private byte[] parseFlag(ByteBuf in) { | ||
|
|
@@ -161,7 +161,7 @@ | |
| final byte[] headerData = new byte[headerLength]; | ||
| in.readBytes(headerData); | ||
| if (log.isDebugEnabled()) { | ||
|
There was a problem hiding this comment. Regarding these modifications to the log printing, you don't have to address them. PR #4698 is currently handling these issues uniformly. 关于日志打印的这些修改，您可以不用处理，PR #4698正在统一处理这些问题。 |
||
| log.debug("Decode headerJson={}", deserializeBytes(headerData)); | ||
|
Check warning on line 164 in eventmesh-common/src/main/java/org/apache/eventmesh/common/protocol/tcp/codec/Codec.java
|
||
| } | ||
| return JsonUtils.parseObject(headerData, Header.class); | ||
| } | ||
|
|
@@ -173,7 +173,7 @@ | |
| final byte[] bodyData = new byte[bodyLength]; | ||
| in.readBytes(bodyData); | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Decode bodyJson={}", deserializeBytes(bodyData)); | ||
|
Check warning on line 176 in eventmesh-common/src/main/java/org/apache/eventmesh/common/protocol/tcp/codec/Codec.java
|
||
| } | ||
| return deserializeBody(deserializeBytes(bodyData), header); | ||
| } | ||
|
|
@@ -227,7 +227,7 @@ | |
| return JsonUtils.parseObject(bodyJsonString, RedirectInfo.class); | ||
| default: | ||
| if (log.isWarnEnabled()) { | ||
| log.warn("Invalidate TCP command: {}", command); | ||
|
Check warning on line 230 in eventmesh-common/src/main/java/org/apache/eventmesh/common/protocol/tcp/codec/Codec.java
|
||
|
There was a problem hiding this comment. The same situation as mentioned earlier: #4702 (comment) 和前面同样的情况：#4702 (comment) |
||
| } | ||
| return null; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe
Integer.BYTESand-9are still not clear enough for developers.Integer.BYTES: The lengths of version field, package length field, and header length field are allInteger.BYTES.-9: Hard-coded.If you disagree with replacing them with expressions composed of more descriptive constant or variable names, as mentioned earlier, you can provide detailed comments to explain the usage of
Integer.BYTESand-9here.我认为
Integer.BYTES和-9对开发者来说依旧不够清晰。Integer.BYTES：Version字段, package length字段, header length字段的长度都是Integer.BYTES。-9：硬编码。如果您不赞同将它们换成有助理解的常量名或变量名组成的表达式，正如我之前所说，您可以用详细的注释来说明这里的
Integer.BYTES和-9。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mxsm I'm curious to understand the thought processes behind choosing a hard-coded value over a dynamic value. I'm wondering about potential future flexibility. Have we considered scenarios where the header structure might evolve, and how a dynamic approach might handle those cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harshithasudhakar For a custom protocol, once it is determined, it will not undergo significant changes for a considerable period of time. This is because any changes to the protocol structure may introduce compatibility issues, potentially requiring existing users to perform a comprehensive upgrade to resolve them. In such cases, a major version upgrade, such as upgrading to version 2.x, may be necessary to undertake protocol modifications. Once the protocol is modified, it will be necessary to reprocess the encoding and decoding of the protocol. Therefore, I believe there is limited need for dynamic handling of these issues. Dynamically setting values increases the complexity of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for clarifying!