Add more information on _failed_to_convert_ exception (#21946) #27034

liketic · 2017-10-17T12:55:23Z

Close #21946

elasticmachine · 2017-10-17T12:55:25Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

DaveCTurner · 2017-10-17T14:06:30Z

@elasticmachine please test this

cbuescher · 2017-10-27T11:51:42Z

core/src/test/java/org/elasticsearch/index/IndexingSlowLogTests.java

@@ -62,6 +64,44 @@ public void testSlowLogParsedDocumentPrinterSourceToLog() throws IOException {
        p = new SlowLogParsedDocumentPrinter(index, pd, 10, true, 3);
        assertThat(p.toString(), containsString("source[{\"f]"));
        assertThat(p.toString(), startsWith("[foo/123] took"));
+
+        source = new BytesReference() {


Is it possible to not implement BytesReference just to throw the exception but to actually use a "source" that will trigger some JsonParseException when creating the parser from source? I think that would be more realistic and a better test.

cbuescher

Thanks, I left a small suggestion for improvement. Also I think @ppf2 who raised the original issue should take a look and let us know his opinion about whether this kind of error message would improve the situation

liketic · 2017-10-28T03:51:27Z

@cbuescher Make sense! Thanks a lot! 😃

cbuescher · 2017-10-30T09:21:05Z

core/src/main/java/org/elasticsearch/index/IndexingSlowLog.java

@@ -193,7 +193,9 @@ public String toString() {
                String source = XContentHelper.convertToJson(doc.source(), reformat, doc.getXContentType());
                sb.append(", source[").append(Strings.cleanTruncate(source, maxSourceCharsToLog)).append("]");
            } catch (IOException e) {
-                sb.append(", source[_failed_to_convert_]");
+                sb.append(", source[_failed_to_convert_[").append(e.getMessage()).append("]]");
+                sb.append(", reformat[").append(reformat).append("]");


Looking at the issue this is trying to fix (#21946), I'm not sure if we need to include the "reformat" and the "x_content_type" in the slow log output.

I pushed 96b88d46cf59a08c9b69e8433364d0c5abc04d30 . Thanks very much! 👍

cbuescher · 2017-10-30T09:23:11Z

@liketic thanks, I left another small question, also I'd like to get some feedback from @ppf2 if he thinks including the exception message improves the situation enough to close #21946.

cbuescher · 2017-10-30T09:23:23Z

@elasticmachine test this please

liketic · 2017-10-31T13:48:14Z

@ppf2 Cloud you take a look at this? 😄

cbuescher

Hi @liketic, looks like @ppf2 approved of the general way to add this error message in the original issue, I just left one comment regarding the test assertion, then I think this is ready to be merged.

cbuescher · 2017-11-03T10:44:28Z

core/src/test/java/org/elasticsearch/index/IndexingSlowLogTests.java

+        pd = new ParsedDocument(new NumericDocValuesField("version", 1), SeqNoFieldMapper.SequenceIDFields.emptySeqID(), "id",
+            "test", null, null, source, XContentType.JSON, null);
+        p = new SlowLogParsedDocumentPrinter(index, pd, 10, true, 3);
+        assertThat(p.toString(), containsString("_failed_to_convert_[Unrecognized token 'invalid'"));


Can you change this assertion to match the whole error string? It might be a bit fragile to future changes in tests, but since we don't randomize anything here the error should stay stable for a bit and this makes it easiert to understand the whole error structure from reading the test.

Thanks! I pushed 9153b6d99567d6b1b68160d739b2656ddf537c74 . We cannot put the while error message here because the internal class org.elasticsearch.common.bytes.BytesReference$MarkSupportingStreamInputWrapper does not implement a toString() so the results of toString() are different each time.

cbuescher · 2017-11-03T13:40:19Z

core/src/test/java/org/elasticsearch/index/IndexingSlowLogTests.java

+            "test", null, null, source, XContentType.JSON, null);
+        p = new SlowLogParsedDocumentPrinter(index, pd, 10, true, 3);
+
+        assertThat(p.toString(), containsString("_failed_to_convert_[Unrecognized token 'invalid':"


Sorry, what I meant by the previous request was to do an assertion on the whole error string (e.g. wie assertEquals), unless there are any reasons preventing this.

Right, I didn't see your comment above. Makes sense to me now, sorry for the noise.

cbuescher

LGTM, I will kick of the CI tests

cbuescher · 2017-11-03T16:05:56Z

@liketic CI doesn't seem happy, although the error doesn't seem to be related to your changes, could you either rebase your branch or merge in master before I kick off another build?

liketic · 2017-11-03T16:24:52Z

@cbuescher Rebase done. Please try again.

cbuescher · 2017-11-03T16:31:46Z

@elasticmachine test this please

cbuescher

Looks ready for merging.

* 6.x: test: Break apart the multi type aspect of rolling upgrade tests, Upgrade to Jackson 2.8.10 (#27230) [Docs] Fix minor paragraph indentation error for multiple Indices params (#25535) Fix inconsistencies in the rest api specs for `tasks` (#27163) Adjust RestHighLevelClient method modifiers (#27238) Add more information on `_failed_to_convert_` exception (#27034) Remove ElasticsearchQueryCachingPolicy (#27190) Backport the size-based index rollver to v6.1.0 Add size-based condition to the index rollover API (#27160) Remove the single argument Environment constructor (#27235) Fix RestGetAction name typo

* master: (25 commits) Disable bwc tests in preparation of backporting #26931 TemplateUpgradeService should only run on the master (#27294) Die with dignity while merging Fix profiling naming issues (#27133) Correctly encode warning headers Fixed references to Multi Index Syntax (#27283) Add an active Elasticsearch WordPress plugin link (#27279) Setting url parts as required to reflect the code base (#27263) keys in aggs percentiles need to be in quotes. (#26905) Align routing param type with search.json (#26958) Update to support bulk updates by query (#27172) Remove duplicated SnapshotStatus (#27276) add split index reference in indices.asciidoc Add ability to split shards (#26931) [Docs] Fix minor paragraph indentation error for multiple Indices params (#25535) Upgrade to Jackson 2.8.10 (#27230) Fix inconsistencies in the rest api specs for `tasks` (#27163) Adjust RestHighLevelClient method modifiers (#27238) Remove unused parameters in AnalysisRegistry (#27232) Add more information on `_failed_to_convert_` exception (#27034) ...

* ccr: (127 commits) Disable bwc tests in preparation of backporting elastic#26931 TemplateUpgradeService should only run on the master (elastic#27294) Die with dignity while merging Fix profiling naming issues (elastic#27133) Correctly encode warning headers Fixed references to Multi Index Syntax (elastic#27283) Add an active Elasticsearch WordPress plugin link (elastic#27279) Setting url parts as required to reflect the code base (elastic#27263) keys in aggs percentiles need to be in quotes. (elastic#26905) Align routing param type with search.json (elastic#26958) Update to support bulk updates by query (elastic#27172) Remove duplicated SnapshotStatus (elastic#27276) add split index reference in indices.asciidoc Add ability to split shards (elastic#26931) [Docs] Fix minor paragraph indentation error for multiple Indices params (elastic#25535) Upgrade to Jackson 2.8.10 (elastic#27230) Fix inconsistencies in the rest api specs for `tasks` (elastic#27163) Adjust RestHighLevelClient method modifiers (elastic#27238) Remove unused parameters in AnalysisRegistry (elastic#27232) Add more information on `_failed_to_convert_` exception (elastic#27034) ...

cbuescher reviewed Oct 27, 2017

View reviewed changes

cbuescher requested changes Oct 27, 2017

View reviewed changes

cbuescher reviewed Oct 30, 2017

View reviewed changes

cbuescher self-assigned this Nov 1, 2017

cbuescher requested changes Nov 3, 2017

View reviewed changes

cbuescher reviewed Nov 3, 2017

View reviewed changes

cbuescher approved these changes Nov 3, 2017

View reviewed changes

Add more information on _failed_to_convert_ exception (elastic#21946)

5d9f0b3

liketic force-pushed the issues/21946 branch from 9153b6d to 5d9f0b3 Compare November 3, 2017 16:23

cbuescher reviewed Nov 6, 2017

View reviewed changes

cbuescher merged commit 5d661df into elastic:master Nov 6, 2017

liketic deleted the issues/21946 branch November 6, 2017 08:49

cbuescher pushed a commit that referenced this pull request Nov 6, 2017

Add more information on _failed_to_convert_ exception (#27034)

67c8048

cbuescher added :Core/Infra/Logging Log management and logging utilities >enhancement v6.1.0 v7.0.0 labels Nov 6, 2017

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more information on _failed_to_convert_ exception (#21946) #27034

Add more information on _failed_to_convert_ exception (#21946) #27034

liketic commented Oct 17, 2017

elasticmachine commented Oct 17, 2017

DaveCTurner commented Oct 17, 2017

cbuescher Oct 27, 2017

cbuescher left a comment

liketic commented Oct 28, 2017

cbuescher Oct 30, 2017

liketic Nov 2, 2017

cbuescher commented Oct 30, 2017

cbuescher commented Oct 30, 2017

liketic commented Oct 31, 2017

cbuescher left a comment

cbuescher Nov 3, 2017

liketic Nov 3, 2017

cbuescher Nov 3, 2017

cbuescher Nov 3, 2017

cbuescher left a comment

cbuescher commented Nov 3, 2017

liketic commented Nov 3, 2017

cbuescher commented Nov 3, 2017

cbuescher left a comment

Add more information on _failed_to_convert_ exception (#21946) #27034

Add more information on _failed_to_convert_ exception (#21946) #27034

Conversation

liketic commented Oct 17, 2017

elasticmachine commented Oct 17, 2017

DaveCTurner commented Oct 17, 2017

cbuescher Oct 27, 2017

Choose a reason for hiding this comment

cbuescher left a comment

Choose a reason for hiding this comment

liketic commented Oct 28, 2017

cbuescher Oct 30, 2017

Choose a reason for hiding this comment

liketic Nov 2, 2017

Choose a reason for hiding this comment

cbuescher commented Oct 30, 2017

cbuescher commented Oct 30, 2017

liketic commented Oct 31, 2017

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher Nov 3, 2017

Choose a reason for hiding this comment

liketic Nov 3, 2017

Choose a reason for hiding this comment

cbuescher Nov 3, 2017

Choose a reason for hiding this comment

cbuescher Nov 3, 2017

Choose a reason for hiding this comment

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher commented Nov 3, 2017

liketic commented Nov 3, 2017

cbuescher commented Nov 3, 2017

cbuescher left a comment

Choose a reason for hiding this comment