Adarsh-Me commented on PR #13566: URL: https://github.com/apache/ignite/pull/13566#issuecomment-5781979680
@zstan @wernerdv could one of you take a look? You are the two most recent people working in the thin-client area (`zstan` assigned #13549 and asked for this PR, and merged #13578; `wernerdv` reviewed and approved it), and this has been sitting without a review since Sep 09. The description now carries the full analysis. Three things worth pulling out: 1. **The behaviour change is two lines.** `isClosed()` stops reporting a metadata result set (`stmt == null`, which is every result set `JdbcThinDatabaseMetadata` builds) as closed, which is what unbreaks `!tables` in SQLLine 1.12.0. 2. **The first commit here was unsafe and I have since fixed it.** `close0()` was using that same `isClosed()` as its guard, so once a metadata result set reported itself open it fell through to `stmt.conn.sendRequest(...)` with `stmt == null` — `rs.close()` threw NPE, on the very result set this PR's own test closes. The second commit rewrites the predicate to `stmt != null && !stmt.isCancelled() && (…)`, which is truth-functionally identical wherever a statement exists. If you only look at the diff, `5bb60777` is the commit worth reading. 3. **I have not built or tested this**, so there is no local green run behind any of it. What I do have is an exhaustive comparison of `close0()` over the 96 reachable field combinations the two methods read, for master vs the first commit vs this head: the first commit adds 10 cursor-close requests that all dereference a null statement, this head adds none and diverges from master nowhere a statement exists. I would like that checked by a real build rather than trusted. Two asks: - **CI**: apart from "Rolling Upgrade check", nothing has ever reported on this PR — including on the first commit, where the NPE would have shown up on the first real build of `JdbcThinResultSetSelfTest`. Could a build be scheduled against `5bb60777` on `ci.ignite.apache.org`? - **Process**: `CONTRIBUTING.md` points code contributions at JIRA (`IGNITE-NNNNN`) and this change has no ticket, only the GitHub issue. I left the title as `Fix #13549` rather than invent a key. Tell me which way this project wants it and I will retitle and reword the commits before anyone spends review time on it. -- 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]
