AlexStocks commented on PR #3253: URL: https://github.com/apache/dubbo-go/pull/3253#issuecomment-4067132441
### Code review Found 3 issues: 1. `filter/README.md` still lists hystrix as a built-in filter after the entire `filter/hystrix/` directory is deleted. Users reading this doc will think hystrix is still available in the main repo. Should remove line 32 or replace it with a note pointing to `dubbo-go-extensions`. https://github.com/apache/dubbo-go/blob/02dc1447d09ef45a05b8473e21612b0443d44b04/filter/README.md#L31-L33 2. `server/options.go` — the PR deletes the todo comment explaining `WithServerFilterConf` was for hystrix, but leaves the function itself with zero documentation. After hystrix removal, there is no indication of what this public API is for, what types `conf any` accepts, or which filters use it. Either add a new doc comment, or remove the function if no other filter needs it (Alanxtl already asked about this). https://github.com/apache/dubbo-go/blob/02dc1447d09ef45a05b8473e21612b0443d44b04/server/options.go#L391-L395 3. No migration guidance for existing hystrix users. The PR description references issue #3242, whose documentation checklist items (README usage guide, dubbo-go-samples example, dubbo-website docs) are all unchecked. Users who depend on `filter/hystrix` will get a broken build with no indication of how to switch to `dubbo-go-extensions`. At minimum, the PR description or a CHANGELOG entry should document the migration path. 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- 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]
