[GitHub] [hadoop-ozone] fapifta commented on pull request #1405: HDDS-4143. Implement a factory for OM Requests that returns an instance based on layout version.

[GitBox <gi...@apache.org>]

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<OzoneManagerProtocolProtos.Type, EnumMap<OMLayoutFeature, T>>, 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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org