fapifta commented on pull request #1405:
URL: https://github.com/apache/hadoop-ozone/pull/1405#issuecomment-690123594
Hi @avijayanhwx, thank you for working on this.
Please find some comments below.
1. First and foremost I would like to comment on the
LayoutVersionInstanceFactory.instances data structure as this data structure
seems to be the most performance critical, because this will be used to look up
the implementation class for every request. I may have an idea to make this a
bit better, which I would like to discuss.
HashMap access is constant time however that constant time does include
hashing.
TreeMap access is O(log(n)).
What we use here in the Map as key is the name of an enum, however this gets
tricky when it comes to ACLs, and the Integer is the LayoutVersion what comes
as well from an enum, however in OMLayoutVersion, we use a separate
layoutVersion, it is still equals to the ordinal of the enum.
So if we change two things:
- OMLayoutVersion to use the enum's ordinal as the LayoutVersion
- ACL requests to be enumerated as separate request types for volume bucket
and key ACL operations, instead of the dynamic string approach for the request
type.
Then we can use an EnumMap>, if we want to make the factory usable by other
parts of the code, then we can supply OzoneManagerProtocolProtos.Type, and
OMLayoutFeature as type parameters to the factory as needed.
With that we would have enummap of enummaps with guaranteed constant time
access (array access basically), which is faster.
Of course this will result in some more memory consumption and a fair bit
more complicated initialization as a tradeoff, because we would need to
register class T to all LayoutVersions for every request type but even with
fairly large request (1-200) and version counts(~1000), the required memory for
these lookup tables are still in the few Mib range, if my estimation skills are
not screwing me over, so it seems to be a fair tradeoff.
Having an enum that enumerates all the requests (ACLs for every type, and
property for every type separately as an abstraction in OM, we can use that in
many other ways possibly, and that also solves the getRequestType()->annotation
conversion, as with the current request types, we can not introduce an
annotation, as for example the "SetAcl.name() + "-" + ObjectType.KEY"
expression does not qualify as a constant and can not be used as value inside
an annotation.
What do you think?
2.
It would be nice to add a test for OMClientRequest, that ensures that it has
and all of its implementations has a declared constructor with an OMRequest
parameter, so that if for some reason this changes we can get a nice detailed
notification about the problem with the change, and based on the test error, we
can either change the OzoneManagerRatisUtils class to call the proper
constructor, or consider the change in the constructor.
3.
I am unsure if we need to restrict BelongsToLayoutFeature to classes and
DisallowedUntilLayoutVersion to methods, what is a reasoning behind such a
restriction? As I see, the code does not understand the annotations in other
places, and probably it is better to restrict, I am just curious if we thought
about methods with features, or completely disallowed classes, in the future?
So this is more like a question than a concern for now.
4.
TestOmRequestFactory does not have a closing empty line.
5.
I am not fully convinced wether it is a consistent and good design decision
to do not have an interface to the LayoutVersionInstanceFactory, and leave
types like OMLayoutVersionIstanceFactory lingering around in the API, are there
any plans to generalize this to an interface -> abstract impl -> concrete impl
structure as with LayoutVersionManager and use the interface only everywhere
where we need to pass it in? Are there any reasons not to do so?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
