[GitHub] ivankelly commented on issue #1281: Issue��#570: Introducing EntryLogManager.

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

ivankelly commented on issue #1281: Issue #570: Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-377207411
 
 
   @sijie 
   
   > I hate to say this, but still I have to say it. because I don't want to get into these kind of arguments about "engineering-preferences" style arguments (e.g. circular dependency, two booleans) again and again.
   
   These are not '"engineering-preferences" style arguments'. boolean parameters and circular dependencies are well established as bad practices.
   
   > I still remembered a similar "circular dependency" argument that lasted for weeks when I introduced a better checkpoint mechanism 3-4 years ago. You disliked whatever you called "circular dependency" in the patch and let me change it to a way you preferred. but the approach turned out to have a correctness issue, and I had to revert it back (#659) recently to my original change that I did for Twitter (which has been running perfectly at twitter production for years). We could have been avoiding this kind of correctness issue if the review was more focused on logic and correctness rather than just thinking of breaking "circular dependency".
   
   Since you brought it up, my objection to BOOKKEEPER-564 was that journal was calling to ledger storage and then ledger storage was then calling to journal. This is _exactly_ where the issue in #659 came from. My initial comments on [BOOKKEPER-564](https://issues.apache.org/jira/browse/BOOKKEEPER-564) were that [the mark should travel with the entries](https://issues.apache.org/jira/browse/BOOKKEEPER-564?focusedCommentId=13623992&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13623992). Anyhow, not to go into too much detail, but if you are trying to use the argument on that JIRA as an example of where arguing for certain engineering standard lead to a bug, you're pretty wide of the mark. SortedLedgerStorage wasn't even in the branch at that point. And what I was arguing against was exactly what caused the bug.
   
   >  Because I see these review comments (around moving classes, breaking circular dependencies) generally are not really helpful. 
   
   If I'm going to review code, I'm going to review it to the best of my ability to ensure that the code is maintainable in the future. If my reviews are unwanted, I can spend my time on other projects.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services