Hi, Thanks everyone for the useful feedback, comments and contributions. All the points/tasks are addressed now and I'm planning to start a vote thread early next week.
Thanks, Rakesh On Tue, May 18, 2021 at 4:08 PM Rakesh Radhakrishnan <rake...@apache.org> wrote: > Hi All, > > It seems that the "performance comparison chart" mentioned in my previous > email is not available to others but it's showing it in my sent items. I am > re-attaching it again with this email, hope it's available to everyone. > Sorry for multiple emails. > > [image: PerfResultChart-KeyCreation-with-diff-layouts.png] > > Thanks, > Rakesh > > On Tue, May 18, 2021 at 1:09 PM Rakesh Radhakrishnan <rake...@apache.org> > wrote: > >> >> Thank you very much @Yiqun for the reply and taking the discussion ahead! >> >> Thanks a lot @Marton for the feedback. We were working on the review >> comments and that caused a delay in my reply. >> >> All the points/tasks are addressed and I've updated the wiki page >> <https://cwiki.apache.org/confluence/display/OZONE/FileSystem+Optimizations+-+HDDS-2939>as >> well, please find my reply to the comments and let me know your feedback. >> >> > @Marton, >> >> > [1] backward compatibility: >> >> > No backward compatibility issues. S3 works well as the feature code sits >> >> > behind the ozone.om.metadata.layout feature flag. Yes, same >> normalization >> >> > behavior as before. Even with feature turned ON the normalization >> behaviour >> >> > is the same, as the configuration "ozone.om.enable.filesystem.paths" >> should >> >> > be set to "true". >> >> >> >>>> Thanks the answer. It turned out that we have backward compatibility >> >> >>>> issues: when the flag is turned on in an existing cluster we can have >> >> >>>> unexpected results. One problem is tracked with HDDS-5094, but --- in >> >> >>>> general -- I think we should be sure that we have no data >> inconsistency >> >> >>>> loss when feature flag is modified in a cluster. Just to having the >> flag >> >> >>>> doesn't save us from all the backward compatibility issues. >> >> >>>> I tested and -- except some backward compatibility issues like >> HDDS-5094 >> >> >>>> -- it worked very well >> >> Thanks for reporting this case - HDDS-5094. We have fixed it and thanks >> again for the patch reviews. >> >> Also, documented that the new metadata layout feature(prefix) is >> supported only in a fresh cluster. >> >> > >> >> > [2] performance: >> >> > When it is turned OFF then there is zero performance penalty because the >> >> > existing functionality is not changed. >> >> > >> >> > With the feature flag turned OFF, master code will create intermediate >> >> > directories during create. When it is turned ON the prefix code also >> does a >> >> > similar approach during create. But, surely we will be focusing on the >> >> > performance path post merge and improving it further. >> >> >>>> My question was about the normal key creation performance when the >> flag >> >> >>>> is turned ON. We shared that the deletion / rename performance is >> >> >>>> significant better, but I guess we have some overhead in the normal >> key >> >> >>>> creation which would be great to know. >> >> Following is the key creation performance comparison chart. When it is >> turned OFF(simple) then there is zero performance penalty because the >> current existing functionality is used. >> >> >> > >> >> > [4] security: >> >> > As you have pointed out, everything works as earlier and there are no >> >> > security implications because of the feature. >> >> >>>> I just learned that we have new code for all the new key creation >> >> >>>> including new ACL checks. So we have new implementation for the >> existing >> >> >>>> functionalities. (I am not sure if we can any test to be more sure, >> but >> >> >>>> it's definitely something which is changed, and -- imho -- even the >> >> >>>> alpha features should be secure on master). >> >> We have added new test cases to verify prefix + ACLs. Both (prefix) new >> and (simple) existing test cases are running fine in CI. >> >> Also, when the feature is turned OFF the existing code path will work as >> earlier. >> >> >>>> I am just wondering what is the long-term plan here. Do we plan to >> >> >>>> handle both prefix/simple code-path with the V1 classes? Or do we >> plan >> >> >>>> to keep both of the classes side by side? Do we plan to avoid >> duplicated >> >> >>>> code, or it's not possible? And what can we do to minimize the new >> >> >>>> technical debts? >> >> This will be addressed as part of the follow up task HDDS-5232. >> >> >>>> While I created HDDS-5106 to use more meaningful names (I don't know >> >> >>>> what is V1, is our current master is the V0?) I am still worried >> about >> >> >>>> this coding style. >> >> Thanks for the suggestions. We have taken care of this and *V1 classes >> have been modified to "WithFSO classes. Existing/Original classes are >> intact. >> >> >> Thanks, >> Rakesh >> >> On Thu, Apr 15, 2021 at 3:26 PM Lin, Yiqun <yiq...@ebay.com.invalid> >> wrote: >> >>> > I am just wondering what is the long-term plan here. Do we plan to >>> handle both prefix/simple code-path with the V1 classes? Or do we >>> plan >>> to keep both of the classes side by side? Do we plan to avoid >>> duplicated >>> code, or it's not possible? And what can we do to minimize the new >>> technical debts? >>> I reviewed most of the feature code during past time. From my personal >>> opinion, it will increases complexity if we support simple/prefix layout at >>> the same time under original logic. As prefix layout uses different table >>> and have the different FS path construction logic. So using a new V1 class >>> for prefix layout implementation will be a good way to push forward on this >>> feature. We already reuse some common code loigc during implementation, but >>> I'm sure there could be more other code lines can be reused. We could have >>> the further code refactor later I think. Also seems V1 class name looks a >>> little consused and can be renamed to a more readable name if we don't want >>> to remove V1 class. >>> >>> Thanks, >>> Yiqun >>> >>> On 2021/4/14, 7:35 PM, "Elek, Marton" <e...@apache.org> wrote: >>> >>> External Email >>> >>> >>> I strongly recommend for everybody to test the feature and check the >>> code. >>> >>> >>> I tested and -- except some backward compatibility issues like >>> HDDS-5094 >>> -- it worked very well. >>> >>> I also checked the Java code and I am very surprised by the >>> implementation. As far as I understand -- but I can be wrong -- both >>> the >>> production and test implementation is based on duplicating some >>> existing >>> classes with V1 post-fixes with limited code re-usage. >>> >>> While I created HDDS-5106 to use more meaningful names (I don't know >>> what is V1, is our current master is the V0?) I am still worried >>> about >>> this coding style. >>> >>> I believe that making the code more maintainable is very important >>> factor (slightly related task about AWS S3 development and >>> simplicity as >>> a design choice [1]) >>> >>> I am just wondering what is the long-term plan here. Do we plan to >>> handle both prefix/simple code-path with the V1 classes? Or do we >>> plan >>> to keep both of the classes side by side? Do we plan to avoid >>> duplicated >>> code, or it's not possible? And what can we do to minimize the new >>> technical debts? >>> >>> TLDR; I recommend to everybody to check the code (and test the >>> branch). >>> >>> Thanks, >>> Marton >>> >>> >>> [1] >>> >>> https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcacm.acm.org%2Fmagazines%2F2021%2F3%2F250706-a-second-conversation-with-werner-vogels%2Ffulltext&data=04%7C01%7Cyiqlin%40ebay.com%7C7767b46a25be4a660a1508d8ff3974ef%7C46326bff992841a0baca17c16c94ea99%7C0%7C0%7C637539969536472439%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NNWShEngcq5VN%2F93yi9uaTP2QsuzHrdN7%2Fn4LMs7L%2FI%3D&reserved=0 >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@ozone.apache.org >>> For additional commands, e-mail: dev-h...@ozone.apache.org >>> >>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@ozone.apache.org >>> For additional commands, e-mail: dev-h...@ozone.apache.org >>> >>
