Pil0tXia commented on code in PR #4229:
URL: https://github.com/apache/eventmesh/pull/4229#discussion_r1268233061
##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/tcp/client/task/MessageTransferTask.java:
##########
@@ -181,25 +181,20 @@ public void run() {
private CloudEvent addTimestamp(CloudEvent event, Command cmd, long
sendTime) {
if (cmd == RESPONSE_TO_SERVER) {
- event = CloudEventBuilder.from(event)
- .withExtension(EventMeshConstants.RSP_C2EVENTMESH_TIMESTAMP,
- String.valueOf(startTime))
- .withExtension(EventMeshConstants.RSP_EVENTMESH2MQ_TIMESTAMP,
- String.valueOf(sendTime))
- .withExtension(EventMeshConstants.RSP_SEND_EVENTMESH_IP,
-
eventMeshTCPServer.getEventMeshTCPConfiguration().getEventMeshServerIp())
- .build();
+ return getCloudEvent(event, sendTime,
EventMeshConstants.RSP_C2EVENTMESH_TIMESTAMP,
EventMeshConstants.RSP_EVENTMESH2MQ_TIMESTAMP,
+ EventMeshConstants.RSP_SEND_EVENTMESH_IP);
} else {
- event = CloudEventBuilder.from(event)
- .withExtension(EventMeshConstants.REQ_C2EVENTMESH_TIMESTAMP,
- String.valueOf(startTime))
- .withExtension(EventMeshConstants.REQ_EVENTMESH2MQ_TIMESTAMP,
- String.valueOf(sendTime))
- .withExtension(EventMeshConstants.REQ_SEND_EVENTMESH_IP,
-
eventMeshTCPServer.getEventMeshTCPConfiguration().getEventMeshServerIp())
- .build();
+ return getCloudEvent(event, sendTime,
EventMeshConstants.REQ_C2EVENTMESH_TIMESTAMP,
EventMeshConstants.REQ_EVENTMESH2MQ_TIMESTAMP,
+ EventMeshConstants.REQ_SEND_EVENTMESH_IP);
}
- return event;
+ }
+
+ private CloudEvent getCloudEvent(CloudEvent event, long sendTime, String
times, String mq, String ip) {
+ return CloudEventBuilder.from(event)
+ .withExtension(times, String.valueOf(startTime))
+ .withExtension(mq, String.valueOf(sendTime))
+ .withExtension(ip,
eventMeshTCPServer.getEventMeshTCPConfiguration().getEventMeshServerIp())
+ .build();
}
Review Comment:
```suggestion
private CloudEvent buildCloudEventWithTimestamps(CloudEvent event,
String client2EventMeshTime,
String eventMesh2MqTime, long sendTime, String eventMeshIP) {
return CloudEventBuilder.from(event)
.withExtension(client2EventMeshTime, String.valueOf(startTime))
.withExtension(eventMesh2MqTime, String.valueOf(sendTime))
.withExtension(eventMeshIP,
eventMeshTCPServer.getEventMeshTCPConfiguration().getEventMeshServerIp())
.build();
}
```
The constant `RSP_C2EVENTMESH_TIMESTAMP` represents the timestamp of the
response from the client to EventMesh.
The constant `RSP_EVENTMESH2MQ_TIMESTAMP` represents the timestamp of the
response from EventMesh to Message Queue.
The constant `RSP_SEND_EVENTMESH_IP` represents the IP address of the
EventMesh that sends the response.
So in my suggestion, the parameter names will be more readable.
Furthermore, the name `getCloudEvent` does not match the functionality of
this method, as it actually adds some extension fields to the CloudEvent. Thus,
I suggest changing it to `buildCloudEventWithTimestamps`.
At last, I have also adjusted the order of the arguments, placing `sendTime`
after `eventMesh2MqTime`. This makes it more aligned with the order in which
the parameters are filled in the method and thus, more readable.
##########
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/tcp/client/task/MessageTransferTask.java:
##########
@@ -181,25 +181,20 @@ public void run() {
private CloudEvent addTimestamp(CloudEvent event, Command cmd, long
sendTime) {
if (cmd == RESPONSE_TO_SERVER) {
- event = CloudEventBuilder.from(event)
- .withExtension(EventMeshConstants.RSP_C2EVENTMESH_TIMESTAMP,
- String.valueOf(startTime))
- .withExtension(EventMeshConstants.RSP_EVENTMESH2MQ_TIMESTAMP,
- String.valueOf(sendTime))
- .withExtension(EventMeshConstants.RSP_SEND_EVENTMESH_IP,
-
eventMeshTCPServer.getEventMeshTCPConfiguration().getEventMeshServerIp())
- .build();
+ return getCloudEvent(event, sendTime,
EventMeshConstants.RSP_C2EVENTMESH_TIMESTAMP,
EventMeshConstants.RSP_EVENTMESH2MQ_TIMESTAMP,
+ EventMeshConstants.RSP_SEND_EVENTMESH_IP);
Review Comment:
To match the changes in the method signature, it would be better to write
the return statement as follows, putting `sendTime` on the same line as
`RSP_EVENTMESH2MQ_TIMESTAMP` to highlight the corresponding relationship
between these two parameters:
```suggestion
return buildCloudEventWithTimestamps(event,
EventMeshConstants.RSP_C2EVENTMESH_TIMESTAMP,
EventMeshConstants.RSP_EVENTMESH2MQ_TIMESTAMP, sendTime,
EventMeshConstants.RSP_SEND_EVENTMESH_IP);
```
However, in this PR, although we've extracted similar code into a method, we
haven't reduced the number of lines of code or enhanced its readability.
Overall, the modifications in this PR are technically acceptable but not
particularly necessary.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
