szetszwo commented on code in PR #7291:
URL: https://github.com/apache/ozone/pull/7291#discussion_r1793742435
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java:
##########
@@ -124,8 +124,10 @@ public void registerHandler(RequestType type, Object
handler) {
@Override
public SnapshotInfo getLatestSnapshot() {
// Transaction buffer will be null during scm initlialization phase
- return transactionBuffer == null
+ final SnapshotInfo snapshotInfo = transactionBuffer == null
? null : transactionBuffer.getLatestSnapshot();
+ LOG.debug("Latest Snapshot Info {}", snapshotInfo);
+ return snapshotInfo;
Review Comment:
Let's add the message to the setter instead, i.e.
```java
+++
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java
@@ -107,6 +107,7 @@ public SnapshotInfo getLatestSnapshot() {
@Override
public void setLatestSnapshot(SnapshotInfo latestSnapshot) {
+ LOG.info("Set latest Snapshot to {}", latestSnapshot);
this.latestSnapshot.set(latestSnapshot);
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java:
##########
@@ -388,6 +397,7 @@ public void notifyConfigurationChanged(long term, long
index,
@Override
public void pause() {
+ LOG.info("SCMStateMachine is pausing");
Review Comment:
Let's print the id and the life cycle state:
```java
LOG.info("{}: Try to pause from current LifeCycle state {}", getId(),
lc);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -276,13 +283,19 @@ public TransactionContext startTransaction(
return ctxt;
}
- return TransactionContext.newBuilder()
+ TransactionContext ctxt = TransactionContext.newBuilder()
.setClientRequest(raftClientRequest)
.setStateMachine(this)
.setServerRole(RaftProtos.RaftPeerRole.LEADER)
.setLogData(raftClientRequest.getMessage().getContent())
.setStateMachineContext(omRequest)
.build();
+ if (LOG.isDebugEnabled()) {
+ long term = ctxt.getLogEntry() != null ? ctxt.getLogEntry().getTerm() :
-1;
+ long index = ctxt.getLogEntry() != null ? ctxt.getLogEntry().getIndex()
: -1;
+ LOG.debug("Start a new transaction. term = {}, index = {}", term, index);
+ }
Review Comment:
Revert this change since `startTransaction` neither has log entry nor
term-index.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -136,6 +137,8 @@ public void initialize(RaftServer server, RaftGroupId id,
super.initialize(server, id, raftStorage);
this.raftGroupId = id;
storage.init(raftStorage);
+ LOG.info("OzoneManagerStateMachine is initializing. raftPeerId = {},
raftGroupId = {}",
+ server.getId(), raftGroupId);
Review Comment:
- Print peer id first. Let's do it for all the messages, it will be easier
for debugging test output since there are more than one peers.
- Could you also remove `raftGroupId` and replace the usage with
`getGroupId()`?
```java
@@ -88,7 +89,6 @@ public class OzoneManagerStateMachine extends
BaseStateMachine {
new SimpleStateMachineStorage();
private final OzoneManager ozoneManager;
private RequestHandler handler;
- private RaftGroupId raftGroupId;
private volatile OzoneManagerDoubleBuffer ozoneManagerDoubleBuffer;
private final ExecutorService executorService;
private final ExecutorService installSnapshotExecutor;
@@ -134,8 +134,8 @@ public void initialize(RaftServer server, RaftGroupId id,
RaftStorage raftStorage) throws IOException {
getLifeCycle().startAndTransition(() -> {
super.initialize(server, id, raftStorage);
- this.raftGroupId = id;
storage.init(raftStorage);
+ LOG.info("{}: initialize {} with {}", id, getGroupId(),
getLastAppliedTermIndex());
});
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java:
##########
@@ -149,6 +153,11 @@ public CompletableFuture<Message> applyTransaction(
final SCMRatisRequest request = SCMRatisRequest.decode(
Message.valueOf(trx.getStateMachineLogEntry().getLogData()));
+ if (LOG.isDebugEnabled()) {
+ long term = trx.getLogEntry() != null ? trx.getLogEntry().getTerm() :
-1;
+ long index = trx.getLogEntry() != null ? trx.getLogEntry().getIndex()
: -1;
+ LOG.debug("A new transaction is being applied. term = {}, index = {}",
term, index);
Review Comment:
Use `TermIndex.valueOf`:
```java
if (LOG.isDebugEnabled()) {
LOG.debug("applyTransaction {}",
TermIndex.valueOf(trx.getLogEntry()));
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -160,6 +165,8 @@ public void notifyLeaderChanged(RaftGroupMemberId
groupMemberId,
RaftPeerId newLeaderId) {
// Initialize OMHAMetrics
ozoneManager.omHAMetricsInit(newLeaderId.toString());
+ LOG.info("New Leader changes. RaftGroupMemberId = {}, newLeader = {}",
+ groupMemberId, newLeaderId);
Review Comment:
Use a shorter message:
```java
LOG.info("{}: leader changed to {}", groupMemberId, newLeaderId);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -145,6 +148,8 @@ public synchronized void reinitialize() throws IOException {
if (getLifeCycleState() == LifeCycle.State.PAUSED) {
unpause(getLastAppliedTermIndex().getIndex(),
getLastAppliedTermIndex().getTerm());
+ LOG.info("OzoneManagerStateMachine is reinitializing. raftPeerId = {},
raftGroupId = {}, " +
+ "lastAppliedTermIndex = {}", getId(), getGroupId(),
getLastAppliedTermIndex());
Review Comment:
It should not call `getLastAppliedTermIndex()` more than once.
```java
@@ -143,8 +143,9 @@ public void initialize(RaftServer server, RaftGroupId id,
public synchronized void reinitialize() throws IOException {
loadSnapshotInfoFromDB();
if (getLifeCycleState() == LifeCycle.State.PAUSED) {
- unpause(getLastAppliedTermIndex().getIndex(),
- getLastAppliedTermIndex().getTerm());
+ final TermIndex lastApplied = getLastAppliedTermIndex();
+ unpause(lastApplied.getIndex(), lastApplied.getTerm());
+ LOG.info("{}: reinitialize {} with {}", getId(), getGroupId(),
lastApplied);
}
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -303,8 +322,7 @@ public TransactionContext
preAppendTransaction(TransactionContext trx)
if (ozoneManager.getAclsEnabled()
&& !ozoneManager.isAdmin(userGroupInformation)) {
String message = "Access denied for user " + userGroupInformation
- + ". "
- + "Superuser privilege is required to prepare ozone managers.";
+ + ". Superuser privilege is required to prepare ozone managers.";
Review Comment:
Let's change the message to
```java
+ ". Superuser privilege is required to prepare
upgrade/downgrade.";
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java:
##########
@@ -414,6 +424,8 @@ public void reinitialize() throws IOException {
throw new IOException(e);
}
+ LOG.info("SCMStateMachine is reinitializing. newTermIndex = {}",
termIndex);
Review Comment:
Include id
```java
LOG.info("{}: SCMStateMachine is reinitializing. newTermIndex = {}",
getId(), termIndex);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -293,6 +306,12 @@ public TransactionContext
preAppendTransaction(TransactionContext trx)
OzoneManagerPrepareState prepareState = ozoneManager.getPrepareState();
+ if (LOG.isDebugEnabled()) {
+ long term = trx.getLogEntry() != null ? trx.getLogEntry().getTerm() : -1;
+ long index = trx.getLogEntry() != null ? trx.getLogEntry().getIndex() :
-1;
+ LOG.debug("Preprocess a new transaction. term = {}, index = {}", term,
index);
+ }
Review Comment:
Use `TermIndex.valueOf`:
```java
if (LOG.isDebugEnabled()) {
LOG.debug("{}: preAppendTransaction {}", getId(),
TermIndex.valueOf(trx.getLogEntry()));
}
```
--
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]
