Copilot commented on code in PR #10075:
URL: https://github.com/apache/seatunnel/pull/10075#discussion_r2627367533
##########
seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/operation/CancelJobOperation.java:
##########
@@ -22,17 +22,23 @@
import
org.apache.seatunnel.engine.server.serializable.ClientToServerOperationDataSerializerHook;
public class CancelJobOperation extends AbstractJobAsyncOperation {
+ private boolean force;
+
public CancelJobOperation() {
super();
}
- public CancelJobOperation(long jobId) {
+ public CancelJobOperation(long jobId, boolean force) {
super(jobId);
+ this.force = force;
}
@Override
protected PassiveCompletableFuture<?> doRun() throws Exception {
SeaTunnelServer service = getService();
+ if (force) {
+ return service.getCoordinatorService().stopJob(jobId);
+ }
return service.getCoordinatorService().cancelJob(jobId);
}
Review Comment:
The `CancelJobOperation` class adds a new `force` field but does not
override the `writeInternal` and `readInternal` methods to properly serialize
and deserialize this field. When this operation is sent across the network in a
distributed Hazelcast cluster, the `force` field will not be transmitted
correctly, causing the operation to always use the default `false` value on the
receiving node.
To fix this, override both methods as follows:
```java
@Override
protected void writeInternal(ObjectDataOutput out) throws IOException {
super.writeInternal(out);
out.writeBoolean(force);
}
@Override
protected void readInternal(ObjectDataInput in) throws IOException {
super.readInternal(in);
force = in.readBoolean();
}
```
This follows the same pattern used in `SubmitJobOperation` which also
extends `AbstractJobAsyncOperation` and has additional fields.
##########
seatunnel-engine/seatunnel-engine-core/src/main/java/org/apache/seatunnel/engine/core/protocol/codec/SeaTunnelCancelJobCodec.java:
##########
@@ -46,29 +50,45 @@ public final class SeaTunnelCancelJobCodec {
public static final int RESPONSE_MESSAGE_TYPE = 14550017;
private static final int REQUEST_JOB_ID_FIELD_OFFSET =
PARTITION_ID_FIELD_OFFSET + INT_SIZE_IN_BYTES;
- private static final int REQUEST_INITIAL_FRAME_SIZE =
+ private static final int REQUEST_FORCE_FIELD_OFFSET =
REQUEST_JOB_ID_FIELD_OFFSET + LONG_SIZE_IN_BYTES;
+ private static final int REQUEST_INITIAL_FRAME_SIZE =
+ REQUEST_FORCE_FIELD_OFFSET + BOOLEAN_SIZE_IN_BYTES;
private static final int RESPONSE_INITIAL_FRAME_SIZE =
RESPONSE_BACKUP_ACKS_FIELD_OFFSET + BYTE_SIZE_IN_BYTES;
private SeaTunnelCancelJobCodec() {}
- public static ClientMessage encodeRequest(long jobId) {
+ public static class RequestParameters {
+
+ public long jobId;
+
+ public Data jobImmutableInformation;
Review Comment:
The `RequestParameters` class contains a field `jobImmutableInformation`
that is declared but never used anywhere in the codec. This field is not
encoded in `encodeRequest`, not decoded in `decodeRequest`, and serves no
purpose in the request structure. It should be removed to avoid confusion and
maintain clean code.
##########
docs/en/seatunnel-engine/rest-api-v2.md:
##########
@@ -870,6 +880,10 @@ curl --location 'http://127.0.0.1:8080/submit-job/upload'
--form 'config_file=@"
}
```
+**Notes:**
+- If the job status is `DOING_SAVEPOINT` and the savepoint does not complete
successfully, a forced stop(When the `force` option is enabled) will set the
job status to `CANCELED`.
Review Comment:
There's a missing space after "stop" in the sentence. It should read "a
forced stop (When the `force` option is enabled)" with a space between "stop"
and the opening parenthesis for better readability and consistency with
standard English writing conventions.
```suggestion
- If the job status is `DOING_SAVEPOINT` and the savepoint does not complete
successfully, a forced stop (When the `force` option is enabled) will set the
job status to `CANCELED`.
```
##########
docs/en/seatunnel-engine/rest-api-v1.md:
##########
@@ -613,6 +623,10 @@ When we can't get the job info, the response will be:
}
```
+**Notes:**
+- If the job status is `DOING_SAVEPOINT` and the savepoint does not complete
successfully, a forced stop(When the `force` option is enabled) will set the
job status to `CANCELED`.
Review Comment:
There's a missing space after "stop" in the sentence. It should read "a
forced stop (When the `force` option is enabled)" with a space between "stop"
and the opening parenthesis for better readability and consistency with
standard English writing conventions.
```suggestion
- If the job status is `DOING_SAVEPOINT` and the savepoint does not complete
successfully, a forced stop (When the `force` option is enabled) will set the
job status to `CANCELED`.
```
--
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]
