[GitHub] [hadoop-ozone] avijayanhwx commented on pull request #1322: HDDS-3829. Introduce Layout Feature interface in Ozone.

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

avijayanhwx commented on pull request #1322:
URL: https://github.com/apache/hadoop-ozone/pull/1322#issuecomment-679268593


   > Hi @avijayanhwx,
   > 
   > thank you for starting this work, and posting an inital version of the core code for the upgrade support.
   > I have a few general questions and concerns, also added a few comments after a quick review.
   > 
   > In HDFS the layoutversion is a monotonically decreasing number, we chose to use monotonically increasing version numbers, I am unsure why HDFS chose the negative numbers, can there be some hidden considerations, we might go through, before committing to the positive layoutversions and monotonic increase? I haven't found one.
   > 
   > Also, in HDFS one layoutversion covers one feature, would it be better to do not let two feetures associated with one layoutversion number? What is the benefit of having two features mapped to the same layout version? I don't feel good about it, though I don't have a specific example yet where it can cause trouble.
   > 
   > LayoutVersionManager is implemented in a way that it is fully static. I am unsure if this is a good design, I understand the intent that there has to be only one instance of this per component, and seeing it this way it is a reasonable choice to use static a implementation, but it can fire back later when we want to implement tests that involves changing something in this logic, and we can not freely and easily change the behaviour in tests as I fear, also it can introduce invisible interdependencies later that might be hard to detect/factor out. Implementing it in a non-static way does not seem to cause any drawback, even we can be fine with multiple instances for the same component, as the used values will be anyway hardcoded in the real system. What do you think, I would consider making it non-static, as I think it has more possibilities and less limitations later.
   
   
   
   > Hi @avijayanhwx,
   > 
   > thank you for starting this work, and posting an inital version of the core code for the upgrade support.
   > I have a few general questions and concerns, also added a few comments after a quick review.
   > 
   > In HDFS the layoutversion is a monotonically decreasing number, we chose to use monotonically increasing version numbers, I am unsure why HDFS chose the negative numbers, can there be some hidden considerations, we might go through, before committing to the positive layoutversions and monotonic increase? I haven't found one.
   > 
   > Also, in HDFS one layoutversion covers one feature, would it be better to do not let two feetures associated with one layoutversion number? What is the benefit of having two features mapped to the same layout version? I don't feel good about it, though I don't have a specific example yet where it can cause trouble.
   > 
   > LayoutVersionManager is implemented in a way that it is fully static. I am unsure if this is a good design, I understand the intent that there has to be only one instance of this per component, and seeing it this way it is a reasonable choice to use static a implementation, but it can fire back later when we want to implement tests that involves changing something in this logic, and we can not freely and easily change the behaviour in tests as I fear, also it can introduce invisible interdependencies later that might be hard to detect/factor out. Implementing it in a non-static way does not seem to cause any drawback, even we can be fine with multiple instances for the same component, as the used values will be anyway hardcoded in the real system. What do you think, I would consider making it non-static, as I think it has more possibilities and less limitations later.
   
   Regarding the HDFS layout feature versions, I could not find any document on why that was the implementation choice. So, I went with what I felt was more intuitive.
   
   I can change the implementation to have only 1 Layout Feature per version.
   
   I am looking into making the Version Manager class non-static. The only issue I see is that it may be used in many different code areas within a component, and hence may need to be passed around (if it cannot be instantiated without external help).


----------------------------------------------------------------
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