Jackie-Jiang commented on PR #9159: URL: https://github.com/apache/pinot/pull/9159#issuecomment-1205669224
> 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. We may add a regression test to verify all the existing keys still have the same ordinal, but we also need to take extra effort maintaining the test whenever a new key is added. E.g. #8884 can still happen if the previous PR didn't add the new keys to the test We do check duplicate id in the static block. Assigning an id has extra benefits though: - People can change the order of the keys for readability (and that's the main reason why people tend to add new keys in the middle so that similar metadata are grouped together, e.g. `numConsumingSegmentsQueried`, `numConsumingSegmentsProcessed`, `numConsumingSegmentsMatched`) - We may remove the old keys that no longer apply -- 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]
