-
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 5 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,60 @@ | ||
/* | ||
* 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, 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 | ||
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. 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 commentThe reason will be displayed to describe this comment to others. Learn more. If I understand your proposal, the 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. OK. Will only implement internal latency for now in the OpenSearchSink and S3 Sink |
||
public Instant getExternalOriginationTime() { | ||
return this.externalOriginationTime; | ||
} | ||
|
||
@Override | ||
public void release(boolean result) { | ||
System.out.println("======release called==="+result); | ||
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. Should remove this print statement 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. oops. Yes |
||
AcknowledgementSet acknowledgementSet = getAcknowledgementSet(); | ||
if (acknowledgementSet != null) { | ||
acknowledgementSet.release(this, result); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,9 @@ | |
|
||
package org.opensearch.dataprepper.model.event; | ||
|
||
import org.opensearch.dataprepper.model.acknowledgements.AcknowledgementSet; | ||
import java.time.Instant; | ||
|
||
public interface EventHandle { | ||
/** | ||
* releases event handle | ||
|
@@ -14,4 +17,46 @@ public interface EventHandle { | |
* @since 2.2 | ||
*/ | ||
void release(boolean result); | ||
|
||
/** | ||
* sets acknowledgement set | ||
* | ||
* @param acknowledgementSet acknowledgementSet to be set in the event handle | ||
* @since 2.6 | ||
*/ | ||
void setAcknowledgementSet(final AcknowledgementSet acknowledgementSet); | ||
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. Do we need to expose all of this? It might make sense to have a different interface for internal use. Say 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. Not sure I understand this suggestion. Are you suggesting we have 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, we'd have two interfaces:
And we implement it as:
In data-prepper, core we'd need a cast: This would be a good indication that somebody is doing something they shouldn't if we see it outside of data-prepper-core. |
||
|
||
/** | ||
* gets acknowledgement set | ||
* | ||
* @return returns acknowledgementSet from the event handle | ||
* @since 2.6 | ||
*/ | ||
AcknowledgementSet getAcknowledgementSet(); | ||
|
||
/** | ||
* 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 |
---|---|---|
|
@@ -91,18 +91,21 @@ protected JacksonEvent(final Builder builder) { | |
} | ||
|
||
this.jsonNode = getInitialJsonNode(builder.data); | ||
this.eventHandle = new DefaultEventHandle(eventMetadata.getTimeReceived()); | ||
} | ||
|
||
protected JacksonEvent(final JacksonEvent otherEvent) { | ||
this.jsonNode = otherEvent.jsonNode.deepCopy(); | ||
this.eventMetadata = DefaultEventMetadata.fromEventMetadata(otherEvent.eventMetadata); | ||
this.eventHandle = new DefaultEventHandle(eventMetadata.getTimeReceived()); | ||
} | ||
|
||
public static Event fromMessage(String message) { | ||
return JacksonEvent.builder() | ||
JacksonEvent event = JacksonEvent.builder() | ||
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. What does this change accomplish? 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. Not needed. I was trying something. undid the change. Thanks! |
||
.withEventType(EVENT_TYPE) | ||
.withData(Collections.singletonMap(MESSAGE_KEY, message)) | ||
.build(); | ||
return event; | ||
} | ||
|
||
private JsonNode getInitialJsonNode(final Object data) { | ||
|
@@ -152,10 +155,6 @@ public void put(final String key, final Object value) { | |
} | ||
} | ||
|
||
public void setEventHandle(EventHandle handle) { | ||
this.eventHandle = handle; | ||
} | ||
|
||
@Override | ||
public EventHandle getEventHandle() { | ||
return eventHandle; | ||
|
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); | ||
} | ||
} |
This file was deleted.
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.
Can we make this package protected? I'd really like to avoid this getting used as it will be hard to clean-up in the future.
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.
By "package protected" you mean make this as
protected class DefaultEventHandle implements EventHandle, Serializable
?