DanielLeens commented on PR #10857:
URL: https://github.com/apache/seatunnel/pull/10857#issuecomment-4417099333
Thanks for the update here. I pulled the latest head locally and re-reviewed
the full diff against `upstream/dev`. I did not run Maven in this pass; this is
a source-level review of the latest revision.
# What This PR Fixes
- User pain: if the Elasticsearch connector returns an HTTP response body
that is never consumed, the underlying connection does not get released back to
the pool, and repeated requests can eventually starve the client.
- Fix approach: this revision consistently reads the response entity before
returning or throwing, so both success paths and non-200 paths release the
response body correctly.
- One-line summary: the latest head closes the success-path gaps that were
still open in the previous revision, and the main Source / Sink / Catalog paths
now all hit the fix.
# Runtime Chain I Checked
```text
Catalog path
-> ElasticSearchCatalog.createTable()
-> EsRestClient.createIndex()
-> ElasticSearchCatalog.dropTable()
-> EsRestClient.dropIndex()
-> ElasticSearchCatalog.truncateTable()
-> EsRestClient.clearIndexData()
Sink schema-change path
-> ElasticsearchSinkWriter.applySingleSchemaChangeEvent()
-> EsRestClient.addField()
Source read path
-> EsRestClient.bulk()
-> EsRestClient.getDocsFromSqlResult()
-> EsRestClient.getDocsFromScrollRequest()
-> EsRestClient.createPointInTime() / deletePointInTime() /
searchWithPointInTime()
In the latest head, those non-HEAD request paths now consume the response
entity before parsing or throwing.
```
# Findings
- I rechecked the exact success-path gaps from the previous round:
- `createIndex`
- `dropIndex`
- `clearIndexData`
- `addField`
They now all read the response body on the success path as well.
- I also re-scanned the remaining `performRequest()` call sites in
`EsRestClient`. The only path that still does not read an entity is
`checkIndexExist()`, which uses an HTTP `HEAD` request and does not rely on a
response body in the first place.
- The new `EsRestClientTest` coverage is focused but meaningful:
- bulk 200 path
- bulk non-200 path with body propagation
- createIndex 200 path
- createIndex non-200 path with body propagation
I do not see a remaining code-level blocker on the latest head.
# Compatibility / Side Effects
- API / config / defaults: no change.
- Protocol / serialization: no change.
- Performance: there is a small and necessary cost to consuming the response
body, but that is exactly what allows the underlying connection to be released
correctly.
- Resource handling: this is the core improvement of the PR, and the latest
head now covers the main success paths that were still open before.
# Tests / CI
- The targeted unit tests are a good fit for this kind of regression.
- I would still treat the current pending `Build` check as an external gate
to watch before merging, but from the source-review side I do not have a
blocker left on this revision.
### Conclusion: can merge
1. Blocking items
- None from my side on the current head.
2. Suggested non-blocking follow-up
- If you want to make the regression net even tighter later, you could add
the same style of focused test for `dropIndex`, `clearIndexData`, and
`addField`, but I do not consider that a blocker for this PR.
This latest revision closes the main-path leak window that was still open in
the prior review. From a code-review perspective, this looks good to merge once
CI finishes cleanly.
--
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]
