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

Jump to bottom

Github-issue#1048 : s3-sink with local-file buffer implementation. #2645

Merged
merged 7 commits into from
May 16, 2023
Merged

Github-issue#1048 : s3-sink with local-file buffer implementation. #2645

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ab1862c
GitHub-issue#1048 : Rebase the code from DP main branch.
deepaksahu562 May 15, 2023
00e2e5f
GitHub-issue#1048
deepaksahu562 May 15, 2023
dbe2811
GitHub-issue#1048
deepaksahu562 May 15, 2023
1cff631
Merge branch 'opensearch-project:main' into s3-sink-temp-local-file
deepaksahu562 May 16, 2023
45e1242
GitHub-issue#1048 : Incorporated review comments.
deepaksahu562 May 16, 2023
00f59cf
GitHub-issue#1048 : Incorporated review comments.
deepaksahu562 May 16, 2023
c2394dc
GitHub-issue#1048 : Incorporated review comments.
deepaksahu562 May 16, 2023
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
21 changes: 5 additions & 16 deletions .../src/main/java/org/opensearch/dataprepper/plugins/sink/accumulator/BufferTypeOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@
package org.opensearch.dataprepper.plugins.sink.accumulator;

import com.fasterxml.jackson.annotation.JsonCreator;

import java.io.File;
import java.io.IOException;
import java.util.Arrays;
import java.util.Map;
import java.util.stream.Collectors;
Expand All @@ -18,28 +15,20 @@
*/
public enum BufferTypeOptions {

INMEMORY("in_memory", new InMemoryBuffer()),
LOCALFILE("local_file", new Object() {
LocalFileBuffer evaluate() {
try {
return new LocalFileBuffer(File.createTempFile("local", null));
} catch (IOException e) {
return null;
}
}
}.evaluate());
INMEMORY("in_memory", new InMemoryBufferFactory()),
LOCALFILE("local_file", new LocalFileBufferFactory());

private final String option;
private final Buffer bufferType;
private final BufferFactory bufferType;
private static final Map<String, BufferTypeOptions> OPTIONS_MAP = Arrays.stream(BufferTypeOptions.values())
.collect(Collectors.toMap(value -> value.option, value -> value));

BufferTypeOptions(final String option, final Buffer bufferType) {
BufferTypeOptions(final String option, final BufferFactory bufferType) {
this.option = option.toLowerCase();
this.bufferType = bufferType;
}

public Buffer getBufferType() {
public BufferFactory getBufferType() {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is a good change. Thanks!

return bufferType;
}

Expand Down
4 changes: 2 additions & 2 deletions ...main/java/org/opensearch/dataprepper/plugins/sink/accumulator/LocalFileBufferFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,20 @@

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.File;
import java.io.IOException;

public class LocalFileBufferFactory implements BufferFactory {

private static final Logger LOG = LoggerFactory.getLogger(LocalFileBufferFactory.class);
public static final String PREFIX = "local";
public static final String SUFFIX = ".log";
@Override
public Buffer getBuffer() {
File tempFile = null;
Buffer localfileBuffer = null;
try {
tempFile = File.createTempFile(PREFIX, null);
tempFile = File.createTempFile(PREFIX, SUFFIX);
localfileBuffer = new LocalFileBuffer(tempFile);
} catch (IOException e) {
LOG.error("Unable to create temp file ", e);
Expand Down
3 changes: 1 addition & 2 deletions .../java/org/opensearch/dataprepper/plugins/sink/accumulator/LocalFileBufferFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,15 @@ class LocalFileBufferFactoryTest {
void test_localFileBufferFactory_notNull() {
LocalFileBufferFactory localFileBufferFactory = new LocalFileBufferFactory();
Assertions.assertNotNull(localFileBufferFactory);
assertThat(localFileBufferFactory, instanceOf(LocalFileBufferFactory.class));
}

@Test
void test_buffer_notNull() {
LocalFileBufferFactory localFileBufferFactory = new LocalFileBufferFactory();
Assertions.assertNotNull(localFileBufferFactory);
assertThat(localFileBufferFactory, instanceOf(LocalFileBufferFactory.class));
Buffer buffer = localFileBufferFactory.getBuffer();
Assertions.assertNotNull(buffer);
assertThat(buffer, instanceOf(Buffer.class));
Copy link
Member

Choose a reason for hiding this comment

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

You should assert that this is an instance of LocalFileBuffer as well.

assertThat(buffer, instanceOf(LocalFileBuffer.class))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Copy link
Member

Choose a reason for hiding this comment

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

@deepaksahu562 , Did you push the change here? I don't see a new assertion for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlvenable, Earlier, I didn't understand. Now I can understand and modify as requested.

Copy link
Member

Choose a reason for hiding this comment

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

It should be as simple as changing this line to:

assertThat(buffer, instanceOf(LocalFileBuffer.class))

Here it is with a little more context.

Buffer buffer = localFileBufferFactory.getBuffer();
Assertions.assertNotNull(buffer);
assertThat(buffer, instanceOf(LocalFileBuffer.class))

We already know this is a Buffer. That is handled statically by Java. But, we want to know that this is returning the LocalFileBuffer and not an InMemoryBuffer (or some other buffer sub-class).

Copy link
Member

Choose a reason for hiding this comment

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

Please change this line per my latest comment. Then we should be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your clarification, Modified as you suggested.

assertThat(buffer, instanceOf(LocalFileBuffer.class));
}
}
4 changes: 2 additions & 2 deletions ...rc/test/java/org/opensearch/dataprepper/plugins/sink/accumulator/LocalFileBufferTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
class LocalFileBufferTest {

public static final String BUCKET_NAME = UUID.randomUUID().toString();
public static final String KEY = UUID.randomUUID().toString() + ".temp";
public static final String KEY = UUID.randomUUID().toString() + ".log";
public static final String PREFIX = "local";
public static final String SUFFIX = ".temp";
public static final String SUFFIX = ".log";
@Mock
private S3Client s3Client;
private LocalFileBuffer localFileBuffer;
Expand Down