sunnysabor commented on PR #7319: URL: https://github.com/apache/shenyu/pull/7319#issuecomment-5889602708
@Aias00 Thanks for the review and for checking the CI failures. Pushed 866d1e504 with the following changes and clarification: 1. **Invalid request IDs:** `WebsocketCollector.initialSync` now catches `IllegalArgumentException`, logs a warning, and ignores the request before starting synchronization. The regression test covers empty/malformed UUID suffixes, no synchronization or response for those requests, and a subsequent valid request on the same session. 2. **Incremental failures:** This is intentional for increments received **before the matching end frame**, with strict readiness enabled. They modify the same caches as the initial frames, so ignoring an application failure in that window could acknowledge an incomplete application. Such failures still require a fresh attempt; a repeatedly failing configuration in that window can keep the gateway not ready. Added tests for synchronous and deferred incremental failures and successful retry. This remains opt-in and does not affect liveness. 3. **Bounded participation:** The end frame now closes the incremental participation window, even while previously registered application work remains pending. Later increments run outside `InitialSyncApplication` and do not increase the attempt's pending count. A regression test sends 100 later increments while initial work is pending, then verifies readiness opens when the initial work completes without waiting for those increments. Work admitted before the end frame still must complete successfully within the existing timeout; this is a fixed completion boundary, not a transactional snapshot or a guarantee of continuously fresh configuration. 4. **Handler map ownership:** Agreed that this deserves an explicit callout. The instance `EnumMap` fixes cross-client handler replacement: constructing another client must not overwrite the subscribers used to apply and acknowledge the first client's configuration. This matters for the multi-Admin application/completion boundary. Validation on Java 17: - 47 focused tests passed (`WebsocketCollectorTest`, `InitialSyncStateTest`, and the loopback `InitialSyncConnectionTest`). - Full `./mvnw -s /tmp/shenyu-maven-central-settings.xml clean install -Dmaven.javadoc.skip=true` passed: all 235 reactor modules, 16m26s, including tests, Checkstyle and RAT. The temporary settings selects Maven Central. - `git diff --check` passed. Dedicated Kubernetes fault-injection scenarios were not run locally. Please take another look, especially at the explicit pre-end-frame failure policy in point 2. -- 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]
