AlexStocks commented on issue #3453:
URL: https://github.com/apache/dubbo-go/issues/3453#issuecomment-4797016697
I think the current issue body is now reasonably narrowed, but for follow-up
efficiency I’d suggest handling the remaining items in two different buckets
instead of treating them all the same.
### Bucket A: can be picked up directly as engineering fixes
These look concrete enough that someone can work on them without first
designing a brand new repro framework:
1. **Getty config panic path**
- Scope: replace panic-style config handling with returned errors in
`remoting/getty/getty_server.go`
- Expected output: small behavioral fix + focused tests for malformed
config / validation failure
2. **Forking cluster losing-branch cancellation**
- Scope: make sibling requests cancelable once the first accepted result
is chosen in `cluster/cluster/forking/cluster_invoker.go`
- Expected output: context/lifecycle improvement + test proving losing
branches are stopped or bounded
3. **TPS limiter cache cleanup**
- Scope: define lifecycle or eviction policy for
`filter/tps/limiter/method_service.go`
- Expected output: cleanup strategy + tests covering stale entry removal
and no regression for normal limiter behavior
4. **panic/fatal policy audit (small-scope first)**
- Suggested approach: do **not** try to clean all panic/fatal sites in
one PR
- Better split:
- `dubbo.go` / startup lifecycle
- `registry/servicediscovery/...`
- `loader.go`
- `filter/sentinel/filter.go`
### Bucket B: should ideally come with a reproducer / benchmark first
These still look valid, but a stronger proof artifact will make review much
easier:
1. **RoundRobin concurrency skew**
- Best next step: add a concurrency stress test / benchmark showing
distribution drift under parallel selection
- Reason: reviewers will want more than a source-level argument before
accepting a synchronization redesign
2. **Graceful termination hardening**
- Best next step: add tests for timeout / result publication / cleanup
ordering
- Reason: shutdown code is easy to destabilize without a clear safety net
3. **JS runtime pooled-state pollution**
- Best next step: create a focused script-router test proving global
state survives across runs
- Reason: if we can reproduce contamination with a minimal script
example, the fix discussion becomes much easier
### Suggested PR slicing
If different community members pick this up, I’d suggest keeping PRs
narrowly scoped:
- PR 1: Getty config error propagation
- PR 2: Forking mode cancellation semantics
- PR 3: TPS limiter eviction/cleanup
- PR 4: RoundRobin repro + fix
- PR 5: graceful termination hardening
- PR 6: JS runtime isolation
- PR 7+: panic/fatal cleanup by subsystem
That slicing should make review easier than one large “audit cleanup” PR.
If helpful later, this issue can also be converted into a lightweight
tracking issue with child issues per item once owners are identified.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
