szetszwo commented on code in PR #10813:
URL: https://github.com/apache/ozone/pull/10813#discussion_r3742938493
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/export/ExportJob.java:
##########
@@ -69,6 +87,183 @@ public int hashCode() {
}
}
- private ExportJob() {
+ /**
+ * Snapshot of export progress returned to callers. Reads live fields from
the enclosing job.
+ */
+ public final class Status {
+ private Status() {
+ }
+
+ public Id getId() {
+ return id;
+ }
+
+ public ExecutionState getExecutionState() {
+ return executionState;
+ }
+
+ public LifeCycleState getLifeCycleState() {
+ return scope.getLifeCycleState();
+ }
+
+ public ContainerHealthState getHealthState() {
+ return scope.getHealthState();
+ }
+
+ public long getTotalRows() {
+ return totalRows;
+ }
+
+ public long getElapsedMs() {
+ if (startTimeNs <= 0) {
+ return 0;
+ }
+ long endNs = endTimeNs > 0 ? endTimeNs : System.nanoTime();
+ return TimeUnit.NANOSECONDS.toMillis(endNs - startTimeNs);
+ }
+
+ public String getTarPath() {
+ return tarPath;
+ }
+
+ public String getErrorMessage() {
+ return errorMessage;
+ }
+
+ public boolean isTerminal() {
+ return executionState == ExecutionState.SUCCEEDED
+ || executionState == ExecutionState.FAILED;
+ }
+ }
+
+ /**
+ * Job execution state.
+ */
+ public enum ExecutionState {
+ RUNNING,
+ SUCCEEDED,
+ FAILED
+ }
+
+ ExportJob(Id id, ExportScope scope, String timestamp, String tarPath,
ContainerID startContainerId,
+ ExportSizing sizing) {
+ this.id = id;
+ this.scope = scope;
+ this.timestamp = timestamp;
+ this.tarPath = tarPath;
+ this.startContainerId = startContainerId != null ? startContainerId :
ContainerID.valueOf(0);
+ this.sizing = sizing;
+ }
+
+ Id getId() {
+ return id;
+ }
+
+ ExportScope getScope() {
+ return scope;
+ }
Review Comment:
Remove used methods (this one and also others).
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/export/ExportJob.java:
##########
@@ -17,14 +17,32 @@
package org.apache.hadoop.hdds.scm.container.export;
+import java.io.BufferedWriter;
+import java.io.IOException;
import java.util.Objects;
import java.util.UUID;
+import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState;
+import org.apache.hadoop.hdds.scm.container.ContainerHealthState;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
/**
- * Container ID export job identifier.
+ * In-memory state for a container ID export job.
*/
public final class ExportJob {
+ private final Id id;
+ private final ExportScope scope;
+ private final String timestamp;
+ private final ContainerID startContainerId;
+ private final ExportSizing sizing;
+ private volatile String tarPath;
+ private volatile ExecutionState executionState = ExecutionState.RUNNING;
+ private volatile long totalRows;
+ private volatile long startTimeNs;
+ private volatile long endTimeNs;
+ private volatile String errorMessage;
Review Comment:
These fields are all `volatile` but there are not `synchronized` in the
code. So, it probably has concurrent issue.
- Methods like getElapsedMs definitely need synchronized.
- Do not add getter/setter for each field. Think about the relationship
between them. Obvious cases are:
- setErrorMessage(..) should also set executionState to FAILED
- When executionState is SUCCEEDED or FAILED, setting it again is a bug.
They need to be redesigned.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/export/ExportJob.java:
##########
@@ -69,6 +87,183 @@ public int hashCode() {
}
}
- private ExportJob() {
+ /**
+ * Snapshot of export progress returned to callers. Reads live fields from
the enclosing job.
+ */
+ public final class Status {
+ private Status() {
+ }
+
+ public Id getId() {
+ return id;
+ }
+
+ public ExecutionState getExecutionState() {
+ return executionState;
+ }
+
+ public LifeCycleState getLifeCycleState() {
+ return scope.getLifeCycleState();
+ }
+
+ public ContainerHealthState getHealthState() {
+ return scope.getHealthState();
+ }
+
+ public long getTotalRows() {
+ return totalRows;
+ }
+
+ public long getElapsedMs() {
+ if (startTimeNs <= 0) {
+ return 0;
+ }
+ long endNs = endTimeNs > 0 ? endTimeNs : System.nanoTime();
+ return TimeUnit.NANOSECONDS.toMillis(endNs - startTimeNs);
+ }
+
+ public String getTarPath() {
+ return tarPath;
+ }
+
+ public String getErrorMessage() {
+ return errorMessage;
+ }
+
+ public boolean isTerminal() {
+ return executionState == ExecutionState.SUCCEEDED
+ || executionState == ExecutionState.FAILED;
+ }
Review Comment:
Move it to the enum.
```java
public enum ExecutionState {
RUNNING(false),
SUCCEEDED(true),
FAILED(true);
private final boolean terminal;
ExecutionState(boolean terminal) {
this.terminal = terminal;
}
public boolean isTerminal() {
return terminal;
}
}
```
--
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]
