FreeOnePlus commented on PR #220: URL: https://github.com/apache/doris-mcp-server/pull/220#issuecomment-5368894080
Thanks for working on bringing these changes forward. However, this PR is not reviewable or mergeable in its current form. ## The PR description is missing The title is only `Merge upstream 1.0`, and the PR body is completely empty. There is no explanation of: - what problem this PR is solving; - which upstream repository, fork, branch, or commit range the changes came from; - which behaviors are intentionally changed; - which Doris/MCP versions and deployment modes were tested; - what compatibility risks exist; - what test commands and results support the change. For a PR with 9 commits, 13 changed files, and changes across routing, authentication-related flows, connection management, metadata, serialization, logging, deployment configuration, and examples, an empty description is not acceptable. Please provide a complete PR description with motivation, scope, before/after behavior, compatibility impact, source provenance, and verification evidence. ## Too many unrelated changes are mixed into one PR This PR currently combines several independent areas: 1. reverse-proxy `ROUTE_PREFIX` support; 2. connection-pool lifecycle and leak fixes; 3. ADBC result serialization; 4. old-Doris metadata compatibility fallback; 5. logging format, rotation, cleanup, and per-request context; 6. deployment defaults and a new example client. These changes have different risks, regression surfaces, reviewers, and rollback requirements. Combining them makes it difficult to review correctness, verify coverage, identify regressions, or revert one behavior without reverting everything else. Please stop stacking additional fixes into this PR and split the work into focused PRs. A reasonable split would be: - **PR 1:** reverse-proxy route-prefix support, including OAuth, token-management, discovery, redirect, form-action, and single/multi-worker tests; - **PR 2:** connection-pool release/discard fixes, including a real `maxsize=1` blocked-waiter concurrency regression test; - **PR 3:** ADBC serialization, with lossless high-precision `DECIMAL` tests; - **PR 4:** old-version table-comment fallback, with exact table matching and structured permission/backend error tests; - **PR 5:** logging rotation/context changes, including `backup_count=0` and multi-process rollover behavior; - **PR 6:** deployment defaults, documentation, and examples. Each PR should have its own complete description, focused commits, regression tests, and compatibility notes. ## Current technical blockers The corresponding focused PRs also need to address the following issues. ### 1. `ROUTE_PREFIX` only handles inbound paths The outer ASGI routers strip the prefix, but generated redirects, form actions, browser `fetch()` calls, token-management URLs, and OAuth discovery/challenge URLs still use root-absolute paths such as `/doris-login`, `/token/*`, and `/auth/login`. A reverse proxy that only exposes `/doris-mcp/` can reach the first endpoint, but subsequent authentication requests leave the prefix and fail. The prefix check also uses a bare `startswith()`, so `/foo` incorrectly matches `/foo-bar/...`. Please introduce one shared prefix-aware public URL helper, use it for every generated URL, and require an exact path-segment boundary: ```python path == prefix or path.startswith(prefix + "/") ```` Please add end-to-end prefixed tests for both single-worker and multi-worker modes. ### 2. ADBC converts exact `Decimal` values to `float` Converting Doris `DECIMAL` values to IEEE-754 floats silently loses precision. Very large values may also become `Infinity` instead of entering the current exception fallback. Please serialize exact decimal values losslessly, normally as strings under one shared result-serialization contract, and add high-precision and large-magnitude regression cases. ### 3. The old-version table-comment fallback is not an exact lookup `SHOW TABLE STATUS ... LIKE 'table_name'` treats `_` and `%` as wildcards, so a valid Doris table name can match a different table. The fallback helper also catches every exception and returns an empty result, which can hide permission and backend failures and weaken the existing Doris OAuth structured 403/502 error contract. Please: * trigger the fallback only for a verified unknown-column error; * perform an exact table-name match; * propagate fallback-stage authorization and backend failures; * add tests for underscore-containing table names; * add a two-stage failure test such as `1054` followed by `1142`. ### 4. Closing and releasing a bad aiomysql connection does not wake blocked acquirers Calling `release()` on an already-closed aiomysql connection removes it from the used set, but does not schedule the normal pool wakeup path. A request that was already blocked while the pool was full can therefore remain blocked until timeout. Please explicitly handle waiter wakeup and add a real concurrency regression test with `maxsize=1`: 1. acquire the only connection; 2. start a second acquire and verify that it is waiting; 3. discard the first connection; 4. verify that the waiter completes promptly without waiting for the acquisition timeout. ### 5. The logging rollover implementation has unsafe edge cases `backup_count=0` is currently valid, but the new handler still creates timestamped backup files and never prunes them, causing unbounded disk growth. In multi-worker mode, all workers on the same port also write and rotate the same files without inter-process synchronization, which can cause rollover races and log loss. Please preserve correct zero-backup semantics and use a genuinely multiprocess-safe design, such as: * a queue-based single writer; * PID-separated files; * or another handler with inter-process coordination. ### 6. `examples/examples.py` is not suitable in its current form The new file: * is missing the ASF license header; * performs network and SQL operations at import time; * assumes a non-default `/doris-mcp/mcp` endpoint; * hard-codes an environment-specific table. Please add the ASF header and a `__main__` guard, use the default `/mcp` endpoint or configurable input, and use a portable example such as `SELECT 1`. ## Requested next step Please close this PR or convert it into a tracking PR, and submit the actual implementation as independently reviewable PRs. Do not continue adding unrelated fixes to this branch. Each replacement PR should include: * a complete description; * a clearly bounded scope; * source provenance where code is being carried over; * before/after behavior; * compatibility impact; * focused regression tests; * exact verification commands and results. Until the scope is restructured, the PR description is completed, and the blockers above are addressed with focused tests, this PR should not be merged. -- 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]
