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 >
