weiqingy commented on PR #930: URL: https://github.com/apache/flink-agents/pull/930#issuecomment-5188124568
@wenjin272 Thanks for the review. Agreed, and agreed that what the description covers matters more than where it came from. Your four look right. I would add two more. Both came up on #952, which is only 123 lines, so this is not just a big-PR thing. Compatibility impact. Writing it made me look at a file the diff never touches, and I found that a refusal we now store on a message gets sent back to the provider on the next request. Nothing breaks, and Java does the same, but it is a real behavior change that none of the design docs mentioned. The #930 miss that became #936 was the same kind of thing, in a file outside the diff. Behavioral contracts. Listing what the code promises, instead of listing the tests, is what showed me one promise had no test at all. I added it before opening the PR. A list of tests would have looked fine. So six: design intent, key decisions, contracts, failure behavior, a test for each contract, compatibility. Runtime flow and diagrams only for big PRs, as you say. Starting with your four and adding as we go is fine too. #965 is 919 lines, so it should tell us more about the big end. -- 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]
