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.

Sign up for GitHub

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

Std out sink to use event model #599

Merged
merged 7 commits into from
Nov 18, 2021
Merged

Std out sink to use event model #599

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c24d8a8
Added md for Log Ingestion Demo Guide
graytaylor0 Oct 29, 2021
cb35482
More fixes
graytaylor0 Nov 2, 2021
12a65e2
Update stdoutsink to use Event model
graytaylor0 Nov 16, 2021
9c0b20e
Address PR comment
graytaylor0 Nov 16, 2021
018c35c
StdOutSink support for both Event and String
graytaylor0 Nov 17, 2021
03e3673
Rename test variables
graytaylor0 Nov 17, 2021
f37ef25
TEST_RECORDS to testRecords
graytaylor0 Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion data-prepper-plugins/common/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jacocoTestCoverageVerification {
violationRules {
rule { //in addition to core projects rule
limit {
minimum = 0.90
minimum = 0.89
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the threshold as is. There is nothing in this PR that requires us to lower it.

Copy link
Member Author

Choose a reason for hiding this comment

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

You would think so, but the coverage is now .89 after this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran your changes locally addressing the Junit issue and looking at the code coverage results from the build. It is possible to achieve .90. Let's keep it at .90.

Copy link
Member Author

@graytaylor0 graytaylor0 Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I had tried this yesterday and it did fix it. Had bumped it back to 0.90 for the next revision. Interesting little thing there with the 2 different versions of Junit. Thanks for catching it!

}
}
}
Expand Down
12 changes: 5 additions & 7 deletions ...-prepper-plugins/common/src/main/java/com/amazon/dataprepper/plugins/sink/StdOutSink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@

import com.amazon.dataprepper.model.annotations.DataPrepperPlugin;
import com.amazon.dataprepper.model.configuration.PluginSetting;
import com.amazon.dataprepper.model.event.Event;
import com.amazon.dataprepper.model.record.Record;
import com.amazon.dataprepper.model.sink.Sink;

import java.util.Collection;
import java.util.Iterator;

@DataPrepperPlugin(name = "stdout", pluginType = Sink.class)
public class StdOutSink implements Sink<Record<String>> {
public class StdOutSink implements Sink<Record<Event>> {

/**
* Mandatory constructor for Data Prepper Component - This constructor is used by Data Prepper
Expand All @@ -38,11 +38,9 @@ public StdOutSink() {
}

@Override
public void output(final Collection<Record<String>> records) {
final Iterator<Record<String>> iterator = records.iterator();
while (iterator.hasNext()) {
final Record<String> record = iterator.next();
System.out.println(record.getData());
public void output(final Collection<Record<Event>> records) {
for (Record<Event> record : records) {
System.out.println(record.getData().toJsonString());
}
}

Expand Down
39 changes: 33 additions & 6 deletions ...per-plugins/common/src/test/java/com/amazon/dataprepper/plugins/sink/StdOutSinkTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

/*
* SPDX-License-Identifier: Apache-2.0
*
Expand All @@ -12,21 +13,47 @@
package com.amazon.dataprepper.plugins.sink;

import com.amazon.dataprepper.model.configuration.PluginSetting;
import com.amazon.dataprepper.model.event.Event;
import com.amazon.dataprepper.model.event.JacksonEvent;
import com.amazon.dataprepper.model.record.Record;
import org.junit.Test;
import org.junit.jupiter.api.BeforeEach;
Copy link
Contributor

Choose a reason for hiding this comment

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

You are using two different version of Junit now. I believe this is what is impacting your test coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! It's a subtle difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for making it support both! I have one comment on variable names and then I'll be ready to approve. It looks like the 90% coverage is working again, correct?

Yep .90 is working again


import java.util.Arrays;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;

public class StdOutSinkTests {
private static String PLUGIN_NAME = "stdout";

private final String TEST_DATA_1 = "data_prepper";
private final String TEST_DATA_2 = "stdout_sink";
private final Record<String> TEST_RECORD_1 = new Record<>(TEST_DATA_1);
private final Record<String> TEST_RECORD_2 = new Record<>(TEST_DATA_2);
private final List<Record<String>> TEST_RECORDS = Arrays.asList(TEST_RECORD_1, TEST_RECORD_2);
private final Map<String, Object> TEST_DATA_1 = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

I recommend that you make this a non-final and initialize it in the setup. As it is right now, authors will need to clear the map in-between tests.

The same goes for TEST_DATA_2 and TEST_RECORDS.

private final Map<String, Object> TEST_DATA_2 = new HashMap<>();

private final List<Record<Event>> TEST_RECORDS = new ArrayList<>();


@BeforeEach
public void setup() {
TEST_DATA_1.put(UUID.randomUUID().toString(), UUID.randomUUID().toString());
TEST_DATA_2.put(UUID.randomUUID().toString(), UUID.randomUUID().toString());

final Record<Event> TEST_RECORD_1 = new Record<>(JacksonEvent
.builder()
.withEventType("event")
.withData(TEST_DATA_1)
.build());

final Record<Event> TEST_RECORD_2 = new Record<>(JacksonEvent
.builder()
.withEventType("event")
.withData(TEST_DATA_2)
.build());

TEST_RECORDS.add(TEST_RECORD_1);
TEST_RECORDS.add(TEST_RECORD_2);
}

@Test
public void testSimple() {
Expand Down