Copilot commented on code in PR #17726:
URL:
https://github.com/apache/dolphinscheduler/pull/17726#discussion_r2560321052
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-sagemaker/src/main/java/org/apache/dolphinscheduler/plugin/task/sagemaker/SagemakerTask.java:
##########
@@ -126,26 +126,36 @@ public void submitApplication() throws TaskException {
@Override
public void cancelApplication() {
- initPipelineId();
try {
+ initPipelineId();
// stop pipeline
utils.stopPipelineExecution(client, pipelineId);
} catch (Exception e) {
throw new TaskException("cancel application error", e);
+ } finally {
+ // shutdown client
+ client.shutdown();
}
}
@Override
public void trackApplicationStatus() throws TaskException {
- initPipelineId();
- // Keep checking the health status
- exitStatusCode = utils.checkPipelineExecutionStatus(client,
pipelineId);
+ try {
+ initPipelineId();
+ // Keep checking the health status
+ exitStatusCode = utils.checkPipelineExecutionStatus(client,
pipelineId);
+ } catch (Exception e) {
Review Comment:
The exception handling in `trackApplicationStatus()` catches all exceptions
and re-throws them as TaskException, but this loses the original exception type
information. For TaskException specifically, this creates unnecessary exception
wrapping (a TaskException wrapped in another TaskException). Consider checking
if the caught exception is already a TaskException and re-throwing it directly,
only wrapping non-TaskException types.
```suggestion
} catch (Exception e) {
if (e instanceof TaskException) {
throw (TaskException) e;
}
```
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-sagemaker/src/main/java/org/apache/dolphinscheduler/plugin/task/sagemaker/SagemakerTask.java:
##########
@@ -126,26 +126,36 @@ public void submitApplication() throws TaskException {
@Override
public void cancelApplication() {
- initPipelineId();
try {
+ initPipelineId();
// stop pipeline
utils.stopPipelineExecution(client, pipelineId);
} catch (Exception e) {
throw new TaskException("cancel application error", e);
+ } finally {
+ // shutdown client
+ client.shutdown();
Review Comment:
The `client.shutdown()` call in the finally block could throw a
NullPointerException if the client is null (e.g., if `init()` was never called
or failed before creating the client). Consider adding a null check: `if
(client != null) { client.shutdown(); }` to prevent exceptions during cleanup.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-sagemaker/src/main/java/org/apache/dolphinscheduler/plugin/task/sagemaker/SagemakerTask.java:
##########
@@ -126,26 +126,36 @@ public void submitApplication() throws TaskException {
@Override
public void cancelApplication() {
- initPipelineId();
try {
+ initPipelineId();
// stop pipeline
utils.stopPipelineExecution(client, pipelineId);
} catch (Exception e) {
throw new TaskException("cancel application error", e);
+ } finally {
+ // shutdown client
+ client.shutdown();
}
}
@Override
public void trackApplicationStatus() throws TaskException {
- initPipelineId();
- // Keep checking the health status
- exitStatusCode = utils.checkPipelineExecutionStatus(client,
pipelineId);
+ try {
+ initPipelineId();
+ // Keep checking the health status
+ exitStatusCode = utils.checkPipelineExecutionStatus(client,
pipelineId);
+ } catch (Exception e) {
+ throw new TaskException(e.getMessage(), e);
+ } finally {
+ // shutdown client
+ client.shutdown();
+ }
}
/**
* init sagemaker applicationId if null
*/
- private void initPipelineId() {
+ public void initPipelineId() {
Review Comment:
[nitpick] The `initPipelineId()` method visibility was changed from
`private` to `public` solely to enable mocking in tests. This breaks
encapsulation and exposes internal implementation details. Consider using
package-private visibility instead (no access modifier) which would allow test
access while maintaining better encapsulation, or use reflection-based mocking
approaches that don't require changing method visibility.
```suggestion
void initPipelineId() {
```
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-sagemaker/src/main/java/org/apache/dolphinscheduler/plugin/task/sagemaker/SagemakerTask.java:
##########
@@ -126,26 +126,36 @@ public void submitApplication() throws TaskException {
@Override
public void cancelApplication() {
- initPipelineId();
try {
+ initPipelineId();
// stop pipeline
utils.stopPipelineExecution(client, pipelineId);
} catch (Exception e) {
throw new TaskException("cancel application error", e);
+ } finally {
+ // shutdown client
+ client.shutdown();
}
}
@Override
public void trackApplicationStatus() throws TaskException {
- initPipelineId();
- // Keep checking the health status
- exitStatusCode = utils.checkPipelineExecutionStatus(client,
pipelineId);
+ try {
+ initPipelineId();
+ // Keep checking the health status
+ exitStatusCode = utils.checkPipelineExecutionStatus(client,
pipelineId);
+ } catch (Exception e) {
+ throw new TaskException(e.getMessage(), e);
+ } finally {
+ // shutdown client
+ client.shutdown();
Review Comment:
The `client.shutdown()` call in the finally block could throw a
NullPointerException if the client is null (e.g., if `init()` was never called
or failed before creating the client). Consider adding a null check: `if
(client != null) { client.shutdown(); }` to prevent exceptions during cleanup.
--
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]
