Event with handle #4952
Event with handle #4952
Conversation
kkondaka
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
Signed-off-by: Kondaka <[email protected]>
Signed-off-by: Kondaka <[email protected]>
Signed-off-by: Kondaka <[email protected]>
kkondaka
force-pushed
the
event-with-handle
branch
from
7155c44 to
5e30e30
Compare
| * @return returns the builder | ||
| * @since 2.10 | ||
| */ | ||
| public Builder<T> withEventHandle(final EventHandle eventHandle) { |
There was a problem hiding this comment.
This needs to be part of BaseEventBuilder. I'm also tempted to say that we only support it in there by making this particular function package protected, but that may also be a bit of work to accomplish.
There was a problem hiding this comment.
Actually, as this is going to be used primarily by the aggregate processor, I think it would be reasonable to expose this only in BaseEventBuilder. You won't have to change every source. Just change that one processor.
There was a problem hiding this comment.
Do you want me to make this change to use BaseEventBuilder in aggregate processor now or can it be part of a future PR?
There was a problem hiding this comment.
Let's add BaseEventBuilder in this one.
There was a problem hiding this comment.
Or you can make this package protected for this PR. Either way.
| @@ -532,10 +536,11 @@ public static JacksonEvent fromEvent(final Event event) { | |||
| public abstract static class Builder<T extends Builder<T>> { | |||
|
|
|||
| private EventMetadata eventMetadata; | |||
| private Object data; | |||
| protected Object data; | |||
There was a problem hiding this comment.
Why do you need to expose this further?
If you do need it, you should have a protected getter: protected Object getData().
There was a problem hiding this comment.
Yeah, I do need it because I am exposing it to derived classes.
There was a problem hiding this comment.
Actually, I need it so that I can modify it directly from derived classes, getData() does not work. I can add setData() but I think it makes sense to actually use protected member so that derived classes can directly modify it. I think it makes sense in this case. Let me know what you think. The current code is broken in the sense that JacksonMetric has it's own data which is different from parent class's data and that is problematic in some cases.
| @@ -860,6 +875,7 @@ void fromEvent_with_a_JacksonEvent() { | |||
| assertThat(createdEvent, notNullValue()); | |||
| assertThat(createdEvent, not(sameInstance(originalEvent))); | |||
| assertThat(event.getEventHandle(), is(notNullValue())); | |||
| assertTrue(event.getEventHandle() instanceof DefaultEventHandle); | |||
There was a problem hiding this comment.
| assertTrue(event.getEventHandle() instanceof DefaultEventHandle); | |
| assertThat(event.getEventHandle(), instanceOf(DefaultEventHandle.class)); |
This will give a better error message if the type is incorrect. It will include the actual type.
| builder.withEventHandle(eventHandle); | ||
| sum = builder.build(); | ||
| final EventHandle handle = sum.getEventHandle(); | ||
| assertThat(handle, is(sameInstance(eventHandle))); |
There was a problem hiding this comment.
Thanks for using sameInstance!
Signed-off-by: Kondaka <[email protected]>
Signed-off-by: Kondaka <[email protected]>
Signed-off-by: Kondaka <[email protected]>
Description
Added
withEventHandleAPI to JacksonEvent.Builder.This is part of supporting acknowledgements to events generated by aggregate processor
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.