-
Notifications
You must be signed in to change notification settings - Fork 210
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
Modify EventHandle to be created for every event and support internal and external origination times #3546
Modify EventHandle to be created for every event and support internal and external origination times #3546
Changes from all commits
26db9db
3cbef88
5e144bd
680bf65
eeedfb8
a9c3ebd
53b1787
df44704
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.model.event; | ||
|
||
import org.opensearch.dataprepper.model.acknowledgements.AcknowledgementSet; | ||
import java.lang.ref.WeakReference; | ||
|
||
import java.time.Instant; | ||
import java.io.Serializable; | ||
|
||
public class DefaultEventHandle implements EventHandle, InternalEventHandle, Serializable { | ||
private Instant externalOriginationTime; | ||
private final Instant internalOriginationTime; | ||
private WeakReference<AcknowledgementSet> acknowledgementSetRef; | ||
|
||
public DefaultEventHandle(final Instant internalOriginationTime) { | ||
this.acknowledgementSetRef = null; | ||
this.externalOriginationTime = null; | ||
this.internalOriginationTime = internalOriginationTime; | ||
} | ||
|
||
@Override | ||
public void setAcknowledgementSet(final AcknowledgementSet acknowledgementSet) { | ||
this.acknowledgementSetRef = new WeakReference<>(acknowledgementSet); | ||
} | ||
|
||
@Override | ||
public void setExternalOriginationTime(final Instant externalOriginationTime) { | ||
this.externalOriginationTime = externalOriginationTime; | ||
} | ||
|
||
public AcknowledgementSet getAcknowledgementSet() { | ||
if (acknowledgementSetRef == null) { | ||
return null; | ||
} | ||
return acknowledgementSetRef.get(); | ||
} | ||
|
||
@Override | ||
public Instant getInternalOriginationTime() { | ||
return this.internalOriginationTime; | ||
} | ||
|
||
@Override | ||
public Instant getExternalOriginationTime() { | ||
return this.externalOriginationTime; | ||
} | ||
|
||
@Override | ||
public void release(boolean result) { | ||
AcknowledgementSet acknowledgementSet = getAcknowledgementSet(); | ||
if (acknowledgementSet != null) { | ||
acknowledgementSet.release(this, result); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ | |
|
||
package org.opensearch.dataprepper.model.event; | ||
|
||
import java.time.Instant; | ||
|
||
public interface EventHandle { | ||
/** | ||
* releases event handle | ||
|
@@ -14,4 +16,30 @@ public interface EventHandle { | |
* @since 2.2 | ||
*/ | ||
void release(boolean result); | ||
|
||
/** | ||
* sets external origination time | ||
* | ||
* @param externalOriginationTime externalOriginationTime to be set in the event handle | ||
* @since 2.6 | ||
*/ | ||
void setExternalOriginationTime(final Instant externalOriginationTime); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should set the origination time on the EventMetadata. The EventHandle can then get this data from the EventMetadata. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Internal origination time is set in EventMetadata and EventHandle gets it from it. Do you want the external origination time also set in the EventMetadata? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, add this to The |
||
|
||
/** | ||
* gets external origination time | ||
* | ||
* @return returns externalOriginationTime from the event handle. This can be null if it is never set. | ||
* @since 2.6 | ||
*/ | ||
Instant getExternalOriginationTime(); | ||
|
||
/** | ||
* gets internal origination time | ||
* | ||
* @return returns internalOriginationTime from the event handle. | ||
* @since 2.6 | ||
*/ | ||
Instant getInternalOriginationTime(); | ||
|
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.model.event; | ||
|
||
import org.opensearch.dataprepper.model.acknowledgements.AcknowledgementSet; | ||
|
||
public interface InternalEventHandle { | ||
/** | ||
* sets acknowledgement set | ||
* | ||
* @param acknowledgementSet acknowledgementSet to be set in the event handle | ||
* @since 2.6 | ||
*/ | ||
void setAcknowledgementSet(final AcknowledgementSet acknowledgementSet); | ||
|
||
/** | ||
* gets acknowledgement set | ||
* | ||
* @return returns acknowledgementSet from the event handle | ||
* @since 2.6 | ||
*/ | ||
AcknowledgementSet getAcknowledgementSet(); | ||
|
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.model.event; | ||
|
||
import org.opensearch.dataprepper.model.acknowledgements.AcknowledgementSet; | ||
import static org.hamcrest.MatcherAssert.assertThat; | ||
import static org.hamcrest.Matchers.equalTo; | ||
import org.junit.jupiter.api.Test; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
import static org.mockito.Mockito.verify; | ||
import static org.mockito.ArgumentMatchers.any; | ||
import org.mockito.Mock; | ||
|
||
import java.time.Instant; | ||
|
||
class DefaultEventHandleTests { | ||
@Mock | ||
private AcknowledgementSet acknowledgementSet; | ||
|
||
@Test | ||
void testBasic() { | ||
Instant now = Instant.now(); | ||
DefaultEventHandle eventHandle = new DefaultEventHandle(now); | ||
assertThat(eventHandle.getAcknowledgementSet(), equalTo(null)); | ||
assertThat(eventHandle.getInternalOriginationTime(), equalTo(now)); | ||
assertThat(eventHandle.getExternalOriginationTime(), equalTo(null)); | ||
eventHandle.release(true); | ||
} | ||
|
||
@Test | ||
void testWithAcknowledgementSet() { | ||
acknowledgementSet = mock(AcknowledgementSet.class); | ||
when(acknowledgementSet.release(any(EventHandle.class), any(Boolean.class))).thenReturn(true); | ||
Instant now = Instant.now(); | ||
DefaultEventHandle eventHandle = new DefaultEventHandle(now); | ||
assertThat(eventHandle.getAcknowledgementSet(), equalTo(null)); | ||
assertThat(eventHandle.getInternalOriginationTime(), equalTo(now)); | ||
assertThat(eventHandle.getExternalOriginationTime(), equalTo(null)); | ||
eventHandle.setAcknowledgementSet(acknowledgementSet); | ||
eventHandle.release(true); | ||
verify(acknowledgementSet).release(eventHandle, true); | ||
} | ||
|
||
@Test | ||
void testWithExternalOriginationTime() { | ||
Instant now = Instant.now(); | ||
DefaultEventHandle eventHandle = new DefaultEventHandle(now); | ||
assertThat(eventHandle.getAcknowledgementSet(), equalTo(null)); | ||
assertThat(eventHandle.getInternalOriginationTime(), equalTo(now)); | ||
assertThat(eventHandle.getExternalOriginationTime(), equalTo(null)); | ||
eventHandle.setExternalOriginationTime(now.minusSeconds(60)); | ||
assertThat(eventHandle.getExternalOriginationTime(), equalTo(now.minusSeconds(60))); | ||
eventHandle.release(true); | ||
} | ||
} |
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.
For the next PR let's scope down and first only implement internal end to end latency metric. For ddb use case we could potentially use this value for external (https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_streams_StreamRecord.html#DDB-Type-streams_StreamRecord-ApproximateCreationDateTime), so we may not need to make any hasty decisions on users configuring this time. @dlvenable What do you think?
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.
If I understand your proposal, the
Source
will set this value on the Event. We don't need to let the user configure this in the pipeline. But, we could in the future. Right?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.
OK. Will only implement internal latency for now in the OpenSearchSink and S3 Sink