Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default codec and buffer handling in Java stdout output #10673

Closed
wants to merge 1 commit into from

Conversation

danhermann
Copy link
Contributor

Correctly sets the default codec to java_line. Fixes buffer handling for events whose encodings do not fit into the buffer.

@@ -20,7 +20,7 @@
public class Stdout implements Output {

public static final PluginConfigSpec<Codec> CODEC_CONFIG =
PluginConfigSpec.codecSetting("codec", "java-line");
PluginConfigSpec.codecSetting("codec", "java_line");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there some test that could be written to fail without this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codec name is resolved at runtime so a test for this would require Logstash to be running. I've been thinking about how to set up integration tests for Java tests for Java plugins and other Java code, but I haven't come up with anything simple, yet.

@@ -57,7 +57,7 @@ public void output(final Collection<Event> events) {
do {
encodeCompleted = codec.encode(e, encodeBuffer);
outputStream.write(encodeBuffer.array(), encodeBuffer.position(), encodeBuffer.limit());
encodeBuffer.flip();
encodeBuffer.clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was there a reason why flip was used before that doesn't make sense any more? I think I'm missing some context here about what is the issue being solved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just a mistaken use of the Buffer API and not noticed earlier because the default buffer size accommodates most events encountered in testing. The flip method "flips" a buffer from write mode to read mode. In the line above, the buffer has already been read in the outputStream.write call so it needs to be flipped back from read mode to write mode. The compact and clear methods do this. Because the buffer is always read in its entirety, the more efficient clear method can be used here instead of compact.

This behavior can be easily verified in a unit test, so I added one for it.

…for events whose encodings do not fit into the buffer.
Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@danhermann
Copy link
Contributor Author

Thanks, @jsvd!

@elasticsearch-bot
Copy link

Dan Hermann merged this into the following branches!

Branch Commits
6.7 8199115
7.0 101e60f
7.x 25158bd
master 03df131

elasticsearch-bot pushed a commit that referenced this pull request Apr 10, 2019
…for events whose encodings do not fit into the buffer.

Fixes #10673
elasticsearch-bot pushed a commit that referenced this pull request Apr 10, 2019
…for events whose encodings do not fit into the buffer.

Fixes #10673
elasticsearch-bot pushed a commit that referenced this pull request Apr 10, 2019
…for events whose encodings do not fit into the buffer.

Fixes #10673
@danhermann danhermann deleted the fix_java_stdout branch April 10, 2019 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants