sijie commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-377198776 @ivankelly > I'm of the opposite view. If the responsibility of each class is not clear, then you cannot possibly have a clean interface abstraction. 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. 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". I am raising this old case here is not to point fingers. I want us to learn a lesson there, and avoid such case happening again. Because I see these review comments (around moving classes, breaking circular dependencies) generally are not really helpful. first, they make reviews losing focus; second, they have nothing to help with engineering progress; third, it exhausts energies for everyone involving in the PR - the author has the change his mind to one reviewer's mind, the other reviewers have to change their also, and etc. so I would be much happier to see if the reviews made are more focused on finding the correctness/logic issues with how the methods defined in this interface, rather than getting into yet-another-styleish-argument.
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services
