xxubai commented on code in PR #4116:
URL: https://github.com/apache/amoro/pull/4116#discussion_r2924295772
##########
amoro-ams/src/main/java/org/apache/amoro/server/process/ProcessService.java:
##########
@@ -249,37 +216,35 @@ private void executeOrTraceProcess(TableProcess process) {
"Submit table process {} to engine {}, process id:{}",
process,
executeEngine.engineType(),
- process.getId());
+ store.getProcessId());
}
/**
* Cancel the given process and untrack it.
*
* @param process table process
*/
- private void cancelProcess(TableProcess process) {
+ private void cancelProcess(TableProcessStore store, TableProcess process) {
- process
- .store()
- .tryTransitState(
- ProcessStatus.CANCELED,
- ProcessEvent.CANCEL_REQUESTED,
- process.getExternalProcessIdentifier(),
- "Gracefully Canceled.",
- process.getProcessParameters(),
- process.getSummary());
-
untrackTableProcessInstance(process.getTableRuntime().getTableIdentifier(),
process.getId());
+ store.tryTransitState(
+ ProcessStatus.CANCELED,
+ ProcessEvent.CANCEL_REQUESTED,
+ store.getExternalProcessIdentifier(),
+ "Gracefully Canceled.",
+ process.getProcessParameters(),
+ process.getSummary());
+ untrackTableProcessInstance(
+ process.getTableRuntime().getTableIdentifier(), store.getProcessId());
- ExecuteEngine executeEngine =
-
executeEngines.get(EngineType.of(process.store().getExecutionEngine()));
+ ExecuteEngine executeEngine =
executeEngines.get(process.getExecutionEngine());
Review Comment:
```suggestion
ExecuteEngine executeEngine =
executeEngines.get(store.getExecutionEngine());
```
While currently the values should match, using different sources is fragile.
Suggest always using store.getExecutionEngine() for consistency.
##########
amoro-ams/src/main/java/org/apache/amoro/server/process/ProcessService.java:
##########
@@ -430,15 +393,15 @@ private void trackTableProcess(
@VisibleForTesting
public TableProcess untrackTableProcessInstance(
ServerTableIdentifier serverTableIdentifier, long processId) {
- Map<Long, TableProcess> inner =
activeTableProcess.get(serverTableIdentifier);
+ Map<Long, TableProcessHolder> inner =
activeTableProcess.get(serverTableIdentifier);
if (inner == null) {
return null;
}
- TableProcess removed = inner.remove(processId);
+ TableProcessHolder removed = inner.remove(processId);
if (inner.isEmpty()) {
activeTableProcess.remove(serverTableIdentifier, inner);
}
- return removed;
+ return removed.getProcess();
Review Comment:
```suggestion
return removed != null ? removed.getProcess() : null;
```
##########
amoro-ams/src/main/java/org/apache/amoro/server/process/ProcessService.java:
##########
@@ -249,37 +216,35 @@ private void executeOrTraceProcess(TableProcess process) {
"Submit table process {} to engine {}, process id:{}",
process,
executeEngine.engineType(),
- process.getId());
+ store.getProcessId());
}
/**
* Cancel the given process and untrack it.
*
* @param process table process
*/
- private void cancelProcess(TableProcess process) {
+ private void cancelProcess(TableProcessStore store, TableProcess process) {
- process
- .store()
- .tryTransitState(
- ProcessStatus.CANCELED,
- ProcessEvent.CANCEL_REQUESTED,
- process.getExternalProcessIdentifier(),
- "Gracefully Canceled.",
- process.getProcessParameters(),
- process.getSummary());
-
untrackTableProcessInstance(process.getTableRuntime().getTableIdentifier(),
process.getId());
+ store.tryTransitState(
+ ProcessStatus.CANCELED,
+ ProcessEvent.CANCEL_REQUESTED,
+ store.getExternalProcessIdentifier(),
+ "Gracefully Canceled.",
+ process.getProcessParameters(),
+ process.getSummary());
+ untrackTableProcessInstance(
+ process.getTableRuntime().getTableIdentifier(), store.getProcessId());
- ExecuteEngine executeEngine =
-
executeEngines.get(EngineType.of(process.store().getExecutionEngine()));
+ ExecuteEngine executeEngine =
executeEngines.get(process.getExecutionEngine());
- executeEngine.tryCancelTableProcess(process,
process.getExternalProcessIdentifier());
+ executeEngine.tryCancelTableProcess(process,
store.getExternalProcessIdentifier());
Review Comment:
executeEngines.get(...) may return null if engine is not registered, leading
to NPE on the next call. Same risk in executeOrTraceProcess.
--
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]
