SEZ9 commented on PR #12023: URL: https://github.com/apache/seatunnel/pull/12023#issuecomment-5738888676
Thanks @zhangshenghang for the fix, for re-running CI on the fork (run 33401475865) and for explaining the stale-check situation. The Build check on head `27ee284b` now shows SUCCESS (run 99524505215), and @nzw921rx has approved as well. The change looks right to me: a `MASTER`-only node never starts `TaskExecutionService`, so returning early in `SendConnectorJarToMemberNodeOperation.run()` and `DeleteConnectorJarInExecutionNode.run()` is the correct guard. It also fixes the case where a single master-only member breaks connector-JAR distribution for the whole cluster, since the broadcast loop `join()`s synchronously on each member. Two small, non-blocking points that can be handled in a follow-up: 1. Please double-check that the `Fixes` reference in the description is meant to fully close that issue. If the issue covers more than the NPE on master-only nodes, a "Relates to" reference would avoid closing it prematurely. 2. The early returns are silent no-ops. A low-level log in both guards (e.g. "skip connector jar operation on master-only node") would make this easier to debug. The shared-config test-isolation nit mentioned earlier is likewise a follow-up. Nothing here blocks merging; I only have comment rights, so a write-capable maintainer will need to perform the actual merge. <!-- streview-comment:1169 --> -- 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]
