wy471x opened a new pull request, #7392: URL: https://github.com/apache/shenyu/pull/7392
Make sure that: - [X] You have read the [contribution guidelines](https://shenyu.apache.org/community/contributor-guide). - [X] You submit test cases (unit or integration tests) that back your changes. - [X] Your local test passed `./mvnw clean install -Dmaven.javadoc.skip=true`. (run as `./mvnw -pl shenyu-kubernetes-controller -am install -DskipTests` for the deps plus `./mvnw -pl shenyu-kubernetes-controller test`: 36 tests, 0 failures, checkstyle and apache-rat passed) ## Summary Follow-up to the review of #7287, related to #6493. The review left two items open, both are addressed here. The issue is already closed by #7287, so this PR only references it. ### Changes: 1. `ServiceIngressCache#putIngressName` (ServiceIngressCache.java:73) - the per-service relation list is now a `CopyOnWriteArrayList`. `getIngressName` (ServiceIngressCache.java:57) returned the list instance stored in the cache while this method (`removeIf` + `add` inside `compute`) and `removeSpecifiedIngressName` (ServiceIngressCache.java:101) mutated that same instance. `IngressControllerConfiguration` builds both controllers with `withWorkerCount(2)` (lines 102 and 141), so `EndpointsReconciler#reconcile` (EndpointsReconciler.java:124) iterating the returned list raced with `IngressReconciler#reconcile` updating the cache. Reproduced with the call shape of both reconcilers: 20/20 runs failed with `ConcurrentModificationException`; throttling the writer to ~800 writes/s still failed, with `ConcurrentModificationException` and `NoSuchElementException` (a removal shrinking the list under the iterator). In both cases the reader aborts at its first `next()`, so the endpoints recon cile fails instead of skipping one element. After the change the same harness reports 0 failures over 6 runs and ~25M reads while writing, and the mutator semantics are unchanged: a re-put replaces the relation of the ingress, `removeSpecifiedIngressName` removes only that ingress, `removeAllIngressName` drains the service. 2. `IngressReconciler#parseServiceFromIngress` (IngressReconciler.java:391) - javadoc now states the limitation that the review asked to make explicit: the result is keyed by service name, so when an ingress routes several paths to the same service with different service ports only the port of the first path that references the service is kept. The relation cached for the ingress is therefore per service rather than per path, and an endpoint update rebuilds that single port for every selector of the ingress. ### Test Cases: - `IngressReconcilerMultiPortPathsTest#testOnlyThePortOfTheFirstPathIsCached` (new) - one ingress with `/first-api` -> 8001 and `/second-api` -> 8002 of the same service, reconciled through the real `IngressReconciler`. Asserts that the parser creates two divide selectors but that `ServiceIngressCache` keeps a single relation for the service whose port is 8001, pinning the documented behaviour so a later path-aware change cannot pass unnoticed. - Existing module tests are unchanged: `EndpointsReconcilerTest` keeps covering the multi-port and named-port selection of #7287. Path-aware routing, where every path of an ingress keeps its own port, would be a separate change; this PR only documents the current behaviour and fixes the cache concurrency, as offered in the review. ## Verification - `./mvnw -pl shenyu-kubernetes-controller test` (JDK 21): `Tests run: 36, Failures: 0, Errors: 0`. - checkstyle (bound to the `validate` phase) and `apache-rat:check` (Unapproved: 0) pass for the module. Related to #6493. -- 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]
