richardstartin commented on PR #9159: URL: https://github.com/apache/pinot/pull/9159#issuecomment-1205626564
> @siddharthteotia @richardstartin To add some context here, we had got multiple PRs merged without following the convention, which broke the backward compatibility. It is hard for all the contributors and reviewers to remember the convention, especially when the change is not next to the comment describing how to keep backward compatible. As @FelixGV pointed out, using enum's ordinal is error-prone, and we need to use a way to enforce people to follow the convention, and make it easy for reviewers to catch when the convention is not followed. > > Example PRs that breaks the backward compatibility: #9092 Merged then reverted the next day #8884 Merged but not caught the backward compatibility issue until this PR (already one month!). I actually pointed it out during the [review](https://github.com/apache/pinot/pull/8884#discussion_r905529608), but the fix goes to the wrong class and I didn't get a chance to review it again. This is a proof that we don't have any guard for this convention, except for the comment that is 30 lines away from the actual change. Clearly a very simple regression test can prevent changes like this from getting merged (and from someone accidentally adding a duplicate id, against which there are currently no guards). Code review shouldn’t focus on things which can be automated simply because hardly anyone has (or should have) the attention span to verify trivialities like these. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
