HDDS-10077. Add hsync metadata to hsync'ed keys in OpenKeyTable as well

[smengcl]

What changes were proposed in this pull request?

Currently, only those keys in KeyTable would have metadata HSYNC_CLIENT_ID when those keys have been hsync'ed (and not closed/committed yet). One problem with this is that it makes getExpiredOpenKeys() and listOpenKeys() (HDDS-8830) inefficient by forcing them to look up KeyTable while they could have just used OpenKeyTable solely to determine whether an open key is hsync'ed or not.

Changes

1. During key hsync(), persist metadata HSYNC_CLIENT_ID to OmKeyInfo in OpenKeyTable as well, in addition to KeyTable. Only write when the client ID changes so it doesn't lead to excessive writes. Ideally, only the first hsync() would lead to a OpenKeyTable write of that key.
2. During key close(), remove HSYNC_CLIENT_ID from OpenKeyTable if any, so that HSYNC_CLIENT_ID isn't unnecessarily written to the final key (which is useless when the key is committed/finalized.
3. Adopt the more efficient approach in getExpiredOpenKeys() and listOpenKeys().

What is the link to the Apache JIRA

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

How was this patch tested?

• Existing tests that cover the changes shall pass.
• Add new test testKeyMetadata to check the intended behavior.
  • When a key is newly created, neither OpenKeyTable nor KeyTable should have hsync metadata.
  • When the key is hsync'ed, both OpenKeyTable and KeyTable should have hsync metadata.
  • When the key is hsync'ed again, both OpenKeyTable and KeyTable should still have hsync metadata.
  • When the key is closed, KeyTable entry should no longer have hsync metadata. Key should not exist in OpenKeyTable anymore.
  • All three types of buckets (LEGACY/OBS/FSO) should behave the same.

[jojochuang]

@ashishkumar50

[sumitagrawl]

@smengcl Thanks for working over this, having few query,

1. adding metadata while commit will not resolve missing hsync commit for first time till fsync not called by client. Do we have plan to resolve by adding hsync flag while open keyfile ?

2. Related to performance, this interface will not be called in normal path. additionally checking key existence for open key is very fast. Do this provide any measurable performance improvement?

[smengcl]

Thanks @sumitagrawl for the comments.

@smengcl Thanks for working over this, having few query,

1. adding metadata while commit will not resolve missing hsync commit for first time till fsync not called by client. Do we have plan to resolve by adding hsync flag while open keyfile ?

Which issue are you talking about? If it is another existing jira then it is not in the scope of this PR. I'm not sure I understand your point. The goal is to identify keys that has been hsync'ed in OpenKeyTable without having to seek KeyTable.

If the client hasn't called hsync() on a key, the key won't have any hsync metadata in the first place.

2. Related to performance, this interface will not be called in normal path. additionally checking key existence for open key is very fast. Do this provide any measurable performance improvement?

It's not merely checking key existence in KeyTable, it is extra key look up + extra DB value deserialization + metadata check. Deserialization the large value byte array that stores OmKeyInfo is probably more expensive than DB get(seek).

[sumitagrawl]

@smengcl Have few minor comments

[smengcl]

Regarding the test failure with the last commit:

Error:    TestLeaseRecovery.testGetCommittedBlockLengthTimeout:330->closeIgnoringKeyNotFound:98 expected: <KEY_NOT_FOUND> but was: <KEY_UNDER_LEASE_RECOVERY>

After a few hours of debugging, I found the new test testGetCommittedBlockLengthTimeout (added in HDDS-10044) does NOT throw KEY_NOT_FOUND when forceRecovery == false during stream.close(), because the key still exists in OpenKeyTable when the stream is being closed even without this PR's change. (Note when forceRecovery is true, the open key is indeed gone. Thus will throw KEY_NOT_FOUND.)

This specific test wasn't previously failing because HSYNC_CLIENT_ID was NEVER put to OpenKeyTable before this patch, as I have detailed above already. And now with this PR, KEY_UNDER_LEASE_RECOVERY is thrown here:

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java

Lines 256 to 262 in a15c9ae

	if (omKeyInfo.getMetadata().containsKey(OzoneConsts.LEASE_RECOVERY) &&
	omKeyInfo.getMetadata().containsKey(OzoneConsts.HSYNC_CLIENT_ID)) {
	if (!isRecovery) {
	throw new OMException("Cannot commit key " + dbOpenKey + " with " + OzoneConsts.LEASE_RECOVERY +
	" metadata while recovery flag is not set in request", KEY_UNDER_LEASE_RECOVERY);
	}
	}

So far it looks like a test issue rather than the code issue and I have adjusted the test to make it pass. Pls check if this is the desired behavior @ChenSammi .

[jojochuang]

LGTM

[sumitagrawl]

LGTM

[ChenSammi]

The modification time should be updated for newOpenKeyInfo.

[smengcl]

Good idea. Then the mod time in open key would indicate the first time the key is hsync'ed (since open key will only be updated on the first hsync).

Done.

[ChenSammi]

So far it looks like a test issue rather than the code issue and I have adjusted the test to make it pass. Pls check if this is the desired behavior @ChenSammi .

@smengcl , thanks for the finding. The fix is good.

[ChenSammi]

The last patch looks good， + 1.

[smengcl]

Thanks @sumitagrawl @ChenSammi @jojochuang for reviewing this.