smengcl commented on code in PR #11810:
URL: https://github.com/apache/ozone/pull/11810#discussion_r4396973706
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3DeleteRevokedSTSTokensRequest.java:
##########
@@ -64,7 +76,44 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager, Execut
final OMResponse.Builder omResponse =
OmResponseUtil.getOMResponseBuilder(getOmRequest());
final List<String> originalAccessKeyIds =
request.getOriginalAccessKeyIdList();
- return new S3DeleteRevokedSTSTokensResponse(originalAccessKeyIds,
omResponse.build());
+ final List<Long> revocationTimeMillisList =
request.getRevocationTimeMillisList();
+ final Table<String, Long> table =
ozoneManager.getMetadataManager().getS3RevokedStsTokenTable();
+ final List<String> deletedIds;
+ try {
+ if (revocationTimeMillisList.isEmpty()) {
Review Comment:
nit: STS is not in any release yet (2.1.x and 2.2.x do not have
`S3DeleteRevokedSTSTokensRequest`), so no released OM ever replicated this
request without cutoffs. This branch could be dropped, or the request rejected
with `INVALID_REQUEST`.
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -2670,10 +2670,13 @@ message RevokeSTSTokenResponse {
/**
This will contain a list of originalAccessKeyIds whose revocation entries
should be removed from
- the s3RevokedStsTokenTable.
+ the s3RevokedStsTokenTable. Each originalAccessKeyId is paired with the
revocation cutoff the
+ cleanup scan read at the same index in revocationTimeMillis. An entry is
deleted only if its
+ current cutoff still equals that value.
*/
message DeleteRevokedSTSTokensRequest {
repeated string originalAccessKeyId = 1;
+ repeated uint64 revocationTimeMillis = 2;
Review Comment:
Could this be a single `optional uint64 purgeBeforeMillis = 2;` per request
instead of one cutoff per key? The scan would set it once to the threshold
`shouldCleanup` already uses, and `validateAndUpdateCache` would delete an id
only if `getIfExist(id)` is non-null and older than that threshold. A newer
`RevokeSTSToken` cutoff always comes from the leader's `CLOCK.millis()` in
`preExecute`, so it is never older than the scan's threshold, which gives the
same result as the equality check.
With one value per request there is no list pairing to validate (the size
mismatch branch goes away), the existing batch size accounting can stay as it
was (no `CodedOutputStream` math, no `getRatisByteLimit()` for tests), and the
batching tests in `TestRevokedSTSTokenCleanupService` would not need to be
rewritten. The cache delete markers are needed either way.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/RevokedSTSTokenCleanupService.java:
##########
@@ -136,8 +145,13 @@ public BackgroundTaskResult call() throws Exception {
// Read the metadata manager on every run, since installing a checkpoint
replaces it
final Table<String, Long> revokedStsTokenTable =
ozoneManager.getMetadataManager().getS3RevokedStsTokenTable();
- long deletedInRun = 0;
+ // This is called submittedInRun because the submitted
originalAccessKeyId and revocationTimeCutoffMillis pair
+ // might not actually get deleted if at time of deletion the
revocationTimeCutoffMillis was updated to a later
+ // value
+ long submittedInRun = 0;
Review Comment:
nit: the `deletedInRun` to `submittedInRun` rename and the log message
changes are not needed for the fix. Consider leaving them out to keep the diff
small.
--
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]
