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

Java plugin API (beta) #10232

Merged
merged 33 commits into from
Feb 4, 2019
Merged

Java plugin API (beta) #10232

merged 33 commits into from
Feb 4, 2019

Conversation

danhermann
Copy link
Contributor

Opening a PR to track commits on this branch

Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

Overall, this is absolutely amazing work. I have left a couple comments in-line.

@@ -98,11 +98,11 @@
expect(subject.sprintf("bonjour")).to eq("bonjour")
end

it "should raise error when formatting %{+%s} when @timestamp field is missing" do
it "should not raise error when formatting %{+%s} when @timestamp field is missing" do
Copy link
Member

Choose a reason for hiding this comment

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

Instead of just expecting it not to raise an error, we should set up expectations for what the new behaviour actually is.

@@ -1,50 +0,0 @@
package co.elastic.logstash.api.v0;
Copy link
Member

Choose a reason for hiding this comment

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

I am a little surprised to see the API version being stripped from the package name, since we had discussed the value of having it in previous iterations. Is this because we expect plugin authors to compile their plugins against an exact version of Logstash in order to catch mismatches at compile-time? This feels like a step backwards to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per our conversation, we'll add API version verification to the Java plugin installation process before this features goes GA.

@@ -68,6 +68,11 @@ public String toString() {
return iso8601Formatter.print(this.time);
}

public long toEpochMilli() {
return (new Duration(JAN_1_1970.toDateTime(DateTimeZone.UTC), this.time).getMillis());
Copy link
Member

Choose a reason for hiding this comment

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

public class Stdin implements Input, Consumer<Map<String, Object>> {

private static final Logger logger = LogManager.getLogger(Stdin.class);
private final Logger LOGGER;
Copy link
Member

Choose a reason for hiding this comment

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

The all-caps variable name makes me think that this would be static. Since it is instance-specific, should we go back to calling it logger?

@danhermann
Copy link
Contributor Author

@yaauie, I've made all the changes you requested on this PR with the exception of the API version verification that will be deferred until the next release.

Copy link
Member

@yaauie yaauie 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 danhermann merged commit 48ee998 into master Feb 4, 2019
danhermann added a commit to danhermann/logstash that referenced this pull request Feb 4, 2019
@danhermann
Copy link
Contributor Author

Thanks for the review, @yaauie!

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

2 participants