Close xcontent parsers (partial)#31513
Merged
tomcallahan merged 5 commits intoelastic:masterfrom Jun 24, 2018
Merged
Conversation
tomcallahan
added
>non-issue
:Core/Infra/Core
Collaborator
|
Pinging @elastic/es-core-infra |
rjernst
approved these changes
Jun 21, 2018
Member
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
LGTM. I left a couple minor things I noticed in formatting, but it was relatively easy to review since the changes were kept to only adding the necessary try lines.
| XContentParser actualJson = createParser(XContentType.JSON.xContent(), actual); | ||
| assertEquals(expectedJson.mapOrdered(), actualJson.mapOrdered()); | ||
| try (XContentParser expectedJson = createParser(XContentType.JSON.xContent(), expected); | ||
| XContentParser actualJson = createParser(XContentType.JSON.xContent(), actual)) { |
Member
There was a problem hiding this comment.
nit: please align the second line of the try with the first line
| XContentParser emptyRegistryParser = xcontentType().xContent() | ||
| .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, new byte[] {}); | ||
| try (XContentParser emptyRegistryParser = xcontentType().xContent() | ||
| .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, new byte[] {})) { |
Member
There was a problem hiding this comment.
I would break this at the = if possible, rather than on the dot of a method call.
Contributor
Author
There was a problem hiding this comment.
Unfortunately this will put us over the line length limit
| assertEquals("[3:27] unable to parse RescorerBuilder with name [bad_rescorer_name]: parser not found", e.getMessage()); | ||
| } | ||
|
|
||
| try (XContentParser parser = createParser(rescoreElement)) { |
Member
There was a problem hiding this comment.
There seems to be an extra level of indentation for all the try statements added in this file.
olcbean
reviewed
Jun 22, 2018
| } catch (IllegalArgumentException e) { | ||
| assertEquals("Unexpected field [random]", e.getMessage()); | ||
| try (XContentParser parser = createParser(JsonXContent.jsonXContent, metadata)) { | ||
| try { |
Contributor
There was a problem hiding this comment.
What about removing the nesting of trys here ?
try (XContentParser parser = createParser(JsonXContent.jsonXContent, metadata)) {
MetaData.Builder.fromXContent(parser);
fail();
} catch (IllegalArgumentException e) {
assertEquals("Unexpected field [random]", e.getMessage());
}
colings86
pushed a commit
that referenced
this pull request
Jun 25, 2018
Partial pass at closing XContentParsers in server. This mostly involved adding try-with-resources statements around the usage of XContentParsers.
jasontedor
added a commit
to hub-cap/elasticsearch
that referenced
this pull request
Jun 25, 2018
* elastic/master: (92 commits) Reduce number of raw types warnings (elastic#31523) Migrate scripted metric aggregation scripts to ScriptContext design (elastic#30111) turn GetFieldMappingsResponse to ToXContentObject (elastic#31544) Close xcontent parsers (partial) (elastic#31513) Ingest Attachment: Upgrade Tika to 1.18 (elastic#31252) TEST: Correct the assertion arguments order (elastic#31540) Add get field mappings to High Level REST API Client (elastic#31423) [DOCS] Updates Watcher examples for code testing (elastic#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (elastic#31474) [DOCS] Move monitoring to docs folder (elastic#31477) Core: Combine doExecute methods in TransportAction (elastic#31517) IndexShard should not return null stats (elastic#31528) fix repository update with the same settings but different type (elastic#31458) Fix Mockito trying to mock IOException that isn't thrown by method (elastic#31433) (elastic#31527) Node selector per client rather than per request (elastic#31471) Core: Combine messageRecieved methods in TransportRequestHandler (elastic#31519) Upgrade to Lucene 7.4.0. (elastic#31529) [ML] Add ML filter update API (elastic#31437) Allow multiple unicast host providers (elastic#31509) ...
dnhatn
added a commit
that referenced
this pull request
Jun 26, 2018
* master: ingest: Add ignore_missing property to foreach filter (#22147) (#31578) Fix a formatting issue in the docvalue_fields documentation. (#31563) reduce log level at gradle configuration time [TEST] Close additional clients created while running yaml tests (#31575) Docs: Clarify sensitive fields watcher encryption (#31551) Watcher: Remove never executed code (#31135) Add support for switching distribution for all integration tests (#30874) Improve robustness of geo shape parser for malformed shapes (#31449) QA: Create xpack yaml features (#31403) Improve test times for tests using `RandomObjects::addFields` (#31556) [Test] Add full cluster restart test for Rollup (#31533) Enhance thread context uniqueness assertion [DOCS] Fix heading format errors (#31483) fix writeIndex evaluation for aliases (#31562) Add x-opaque-id to search slow logs (#31539) Watcher: Fix put watch action (#31524) Add package pre-install check for java binary (#31343) Reduce number of raw types warnings (#31523) Migrate scripted metric aggregation scripts to ScriptContext design (#30111) turn GetFieldMappingsResponse to ToXContentObject (#31544) Close xcontent parsers (partial) (#31513) Ingest Attachment: Upgrade Tika to 1.18 (#31252) TEST: Correct the assertion arguments order (#31540)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm working through the codebase trying to close every instance where we leave an XContentParser open... there are a lot of them. I will follow with separate PR's that enforce that we close XContentParsers.
I recommend viewing this PR with &w=1.
If there's too much here, I'm happy to break it up into multiple different PRs.
Relates #30692