-
Notifications
You must be signed in to change notification settings - Fork 15
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/dlq timestamp #7
base: main
Are you sure you want to change the base?
Conversation
864add0
to
5d27419
Compare
@@ -98,7 +101,6 @@ private void writeOffsets(Path segment, long offset) throws IOException { | |||
} | |||
|
|||
public void close() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor - can you add the @Override
annotation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
@@ -34,7 +34,7 @@ | |||
import java.util.function.Consumer; | |||
|
|||
|
|||
public class DeadLetterQueueInputPlugin { | |||
public class DeadLetterQueueInputPlugin implements AutoCloseable{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor/style - since close is throws IOException this can implement Closeable instead of AutoClosable, which (imo) is a bit more idiomatic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong feelings either way - I added the AutoCloseable interface to allow the use of try.. with resources to help avoid resource leaks/clean up tests. My understanding was the AutoCloseable was more used for code targeting JDK7+, but I guess you are right in that the existing close() throws IOException implies Closeable. (That also means that I need to fix up the method to make it idempotent, which it should have been anyway... ;))
5d27419
to
a589b51
Compare
a589b51
to
5e150cf
Compare
5e150cf
to
66d713c
Compare
Its this PR related to fix the CI test on master and the 6.x branch? https://travis-ci.org/logstash-plugins/logstash-input-dead_letter_queue
|
Change behavior to only respect the 'startTimestamp' flag when no offsets have been committed.
66d713c
to
1b82b89
Compare
@ph - the reason the tests are failing on 6.x and master is elastic/logstash#8128 (elastic/logstash@d121c58), which removes the toIso8601 method from timestamp. This PR does remove that method, but a less invasive PR should probably go in between. I'll put it up shortly |
Thanks @robbavey ! |
Stop timestamp overriding committed offsets
Change behavior to only respect the 'startTimestamp' flag when no offsets have been committed.
Adds more tests.
Fixes #6