fmorg-git commented on code in PR #10197:
URL: https://github.com/apache/ozone/pull/10197#discussion_r3400779704
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java:
##########
@@ -692,6 +687,22 @@ private RequestContext
maybeAttachSessionPolicyFromThreadLocal(RequestContext co
.build();
}
+ /**
+ * Attaches s3 action to RequestContext if an S3Authentication is found in
the Ozone Manager thread local,
+ * and it has an s3 action. Otherwise, returns the RequestContext as it was
before.
+ * @param context the original RequestContext
+ * @return RequestContext as before or with s3 action embedded
+ */
+ private RequestContext maybeAttachS3ActionFromThreadLocal(RequestContext
context) {
Review Comment:
> BTW, can we move setS3Action and setSessionPolicy to where context is
created?
originally, I implemented this suggestion but this broke some of my unit
tests in TestOMMetadataReader. Now I remember why I did it the way I did, is
because there are many sites where the RequestContext is created but they all
funnel down to this method in OmMetadataReader:
```
public boolean checkAcls(OzoneObj obj, RequestContext context,
boolean throwIfPermissionDenied)
```
So it's safer to have it in the one central location that everyone uses,
because if we try to rely on adding to all the sites where RequestContext is
created, if a new overloaded call site is added in future, someone would need
to remember to update that one as well or else there would be a bug until it is
realized, which is brittle. The second issue is that someone might call this
method directly like the unit test does, and if we don't set the values here,
it would break, just like the unit test. So if we need to set the values in
this method anyway, we might as well just set it once. I understand it is an
extra object allocation, but at least now the two maybeAttach* methods are
combined into one, so it's one less allocation and the code is more
maintainable with the one central location.
--
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]
