gesterzhou commented on pull request #5775: URL: https://github.com/apache/geode/pull/5775#issuecomment-735441681
I reviewed the code. It's the phase 2 of a good feature. The phase 1 is GEODE-7565. At that time, there's already a potential issue to break backward compatibility. Base on my limited knowledge, I feel that introducing ServerLocationAndMemberId to replace ServerLocation might not be the correct way. The better way should be to introduce a new field "String memberId" into the ServerLocation class, then use toDataPre_GEODE_1_14_0_0 and fromDataPre_GEODE_1_14_0_0 in ServerLocation to handle the new field for rollingupgrade. As for how to find such issue earlier in geode, we do have rollingupgrade dunit tests. The pull request does not have this type of test, then it has to rely on hydra to find the issue. It's not hydra's fault though. We should feel happy that hydra protected us and found the issue today. As for how to cooperate with remote committer, it's a long story. I'd like to cooperate with remote committer to enhance the GEODE-8656. But even I am not skillful to know the root cause and how to fix, as the rule of regression analysis, we should still revert first then re-fix. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
