HDDS-4477. Delete txnId in SCMMetadataStoreImpl may drop to 0 after SCM restart.

[lokeshj1703]

What changes were proposed in this pull request?

If SCM cleans up all the DeletedBlocksTransaction in deletedBlocksTable, then restart, the txID in SCMMetadataStoreImpl will drop to 0.

in SCMMetadataStoreImpl.java

public SCMMetadataStoreImpl(OzoneConfiguration config)
    throws IOException {
  this.configuration = config;
  start(this.configuration);
  this.txID = new AtomicLong(this.getLargestRecordedTXID());
}

private Long getLargestRecordedTXID() throws IOException {
  try (TableIterator<Long, ? extends KeyValue<Long, DeletedBlocksTransaction>>
      txIter = deletedBlocksTable.iterator()) {
    txIter.seekToLast();
    Long txid = txIter.key();
    if (txid != null) {
      return txid;
    }
  }
  return 0L;
}

in DeletedBlockLogImpl.java

public void commitTransactions(
    List<DeleteBlockTransactionResult> transactionResults, UUID dnID) {
  ...
  scmMetadataStore.getDeletedBlocksTXTable().delete(txID);
  ...
}

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4477

How was this patch tested?

Adds UT

[lokeshj1703]

@GlenGeng Can you please take a look?

[amaliujia]

I think currently we are planning to use a distributed sequence ID generator (design doc: https://docs.google.com/document/d/13qSwLuDBw7mytJtF9h2oauuLocFfr-XJSBdTfc1YvxM/edit?usp=sharing)

The idea is we are migrating SCM to Ratis based in HDDS-2823 (so for both HA and non-HA it uses the same code). Thus we implement a ID generator that based on term, which guarantees to be monotonic increasing.

Because of it, I suggest we postpone the fix in master branch for this issue unless it is causing serious production issue.

[GlenGeng-awx]

cc @runzhiwang, he is currently working on integrating DeleteBlockLog with ratis.

This solution is fine to me. Thanks for this smart solution!

In SCM HA, we decided to use the distributed sequence id <term, counter> as the txnId, which can easily maintain monotonically increasing property. Meanwhile in SCM running without ratis, we do not have term, thus need to leverage this solution to provide a monotonically increasing txnId.

As mentioned by @bshashikant, we might need support switch from HA (using Ratis) to non-HA (not using Ratis), it will be hard to maintain the monotonically increasing property between the two kind of txnIds.

For the txnId, since SCM HA will keep the DeletedBlocksTXTable to be the same across SCMs, I consider we don't need the distributed sequence id solution any more, this solution will be sufficient.

What do you think @runzhiwang @bshashikant @amaliujia @nandakumar131 ?

Attention that we might meet same problem for containerID and localID generation if we need to support switch ratis SCM to non-ratis SCM.

[GlenGeng-awx]

How about remove this addTransaction(), since it is only used in unit test ?

[runzhiwang]

@GlenGeng I have done this in my DeleteBlock pr.

[GlenGeng-awx]

LGTM.

[amaliujia]

Yes. I think if we eventually decide to not use Ratis for SCM non-HA, the fix in this PR will still be needed. If we decide to use Ratis for SCM non-HA, the solution for this case might will complicate HDDS-2823 a bit.

I would also like to hear other people's opinion for sure.

[runzhiwang]

@lokeshj1703 maybe we need some comments. Besides can we define a new method to do the following. And because the containerID and count are dummy, we can define a const such as DUMMY_CONTAINERID = 1.

      scmMetadataStore.getDeletedBlocksTXTable()
          .putWithBatch(batch, largestTxnIdHolderKey,
              DeletedBlocksTransaction.newBuilder().setTxID(getCurrentTXID())
                  .setContainerID(1).setCount(1).build());

[lokeshj1703]

Addressed in latest commit.

[lokeshj1703]

@GlenGeng @amaliujia @runzhiwang Thanks for reviewing the PR!

For the txnId, since SCM HA will keep the DeletedBlocksTXTable to be the same across SCMs, I consider we don't need the distributed sequence id solution any more, this solution will be sufficient.

I agree with @GlenGeng . Since delete blocks table would be consistent across all SCM instances, we might not need DistributedSequenceIdGenerator for delete transactionId.

[bshashikant]

@GlenGeng @amaliujia @runzhiwang Thanks for reviewing the PR!

For the txnId, since SCM HA will keep the DeletedBlocksTXTable to be the same across SCMs, I consider we don't need the distributed sequence id solution any more, this solution will be sufficient.

I agree with @GlenGeng . Since delete blocks table would be consistent across all SCM instances, we might not need DistributedSequenceIdGenerator for delete transactionId.

I agree with @lokeshj1703 and @GlenGeng here. This seems to be better approach to handle delete transactionId for both ratis and non-ratis case. I would prefer to get this patch committed.

[amaliujia]

Sure I agree for for TxnID we can follow the approach in this PR.

To make such approach be safe for SCM HA on Ratis, we will need follower installSnapshot implementation. That is on my TODO list in the near future.

[runzhiwang]

LGTM. Need to fix checkstyle

[lokeshj1703]

@bshashikant Thanks for bringing up upgrade compatibility in internal discussion!

I have added a commit to address checkstyle issues and an upgrade fix. For upgrade, the old table would not have largestTxnIdHolderKey and therefore we would need to fetch the largest transaction from the table if it exists.

[bshashikant]

Thanks @lokeshj1703 for the contribution.