sumitagrawl commented on code in PR #5891:
URL: https://github.com/apache/ozone/pull/5891#discussion_r1440136117
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -493,15 +489,28 @@ public OzoneManagerDoubleBuffer
buildDoubleBufferForRatis() {
*/
@Override
public long takeSnapshot() throws IOException {
- LOG.info("Current Snapshot Index {}", getLastAppliedTermIndex());
- TermIndex lastTermIndex = getLastAppliedTermIndex();
- final TransactionInfo build = TransactionInfo.valueOf(lastTermIndex);
- ozoneManager.setTransactionInfo(build);
- Table<String, TransactionInfo> txnInfoTable =
- ozoneManager.getMetadataManager().getTransactionInfoTable();
- txnInfoTable.put(TRANSACTION_INFO_KEY, build);
+ try {
+ ozoneManagerDoubleBuffer.awaitFlush();
+ } catch (InterruptedException e) {
+ throw IOUtils.toInterruptedIOException("Interrupted
ozoneManagerDoubleBuffer.awaitFlush", e);
+ }
+
+ return takeSnapshotImpl();
+ }
+
+ private synchronized long takeSnapshotImpl() throws IOException {
+ final TermIndex applied = getLastAppliedTermIndex();
+ final TermIndex notified = getLastNotifiedTermIndex();
+ final TermIndex snapshot = applied.compareTo(notified) > 0 ? applied :
notified;
Review Comment:
updating DB with notified TermIndex will be incorrect, as notified will be
greater than applied, as there can be many transaction pending in executers and
may take time. So this update can give wrong snapshot info.
Further if there is a restart, then raft log not yet executed will never be
executed as transaction info in DB is updated to the latest.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -186,19 +177,26 @@ public void notifyLeaderChanged(RaftGroupMemberId
groupMemberId,
* @param index index which is being updated
*/
@Override
- public void notifyTermIndexUpdated(long currentTerm, long index) {
- // SnapshotInfo should be updated when the term changes.
- // The index here refers to the log entry index and the index in
- // SnapshotInfo represents the snapshotIndex i.e. the index of the last
- // transaction included in the snapshot. Hence, snaphsotInfo#index is not
- // updated here.
+ public synchronized void notifyTermIndexUpdated(long currentTerm, long
index) {
+ final TermIndex newTermIndex = TermIndex.valueOf(currentTerm, index);
+ lastNotifiedTermIndex = assertUpdateIncreasingly("lastNotified",
lastNotifiedTermIndex, newTermIndex);
Review Comment:
I have seen issues while handling with earlier PR, that, leader election
depends on notifiedTermIndex (as updated to lastAppliedTermIndex), but here we
are not updating to lastAppliedTermIndex.
Check if this can have impact to ratis, as this information is not known to
ratis internally till snapshot is taken.
--
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]
