-
Notifications
You must be signed in to change notification settings - Fork 3
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
archiving of existing events #9
archiving of existing events #9
Conversation
when(event.metadata()).thenReturn(metadata); | ||
when(event.metadata().name()).thenReturn("sample.archive.events.name"); | ||
|
||
assertTrue(sampleTransformation.action(event)==ARCHIVE); |
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.
formatting around the == and in the other test.
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.
Accepted and done
@@ -98,6 +107,12 @@ private boolean eventStoreTransformedEventPresent() throws SQLException { | |||
|
|||
private void insertEventLogData() throws SQLException, InvalidSequenceIdException { |
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.
Not sure if it would be cleaner if we had a method to insert event for stream, at version with name. But this is ok.
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.
Done, refactored method to have parameters and removed added method.
return false; | ||
} | ||
|
||
default TransformAction action(final JsonEnvelope event) { |
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.
java doc comments. Also update the deprecated comment to point to #action()
Is actionFor a better name than action ? (just asking the question, can discuss)
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.
updated java doc and renamed action method to actionFor
@@ -17,15 +21,25 @@ | |||
* @param event - the event to check | |||
* @return TRUE if the event is eligible to have the transformation applied to it. |
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.
should probably update javadoc to mention default implementation is false also
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.
Done
|
||
/** | ||
* Transforms an events into zero to many events. | ||
* | ||
* @param event - the event to be transformed | ||
* @return a stream of transformed events. | ||
*/ | ||
Stream<JsonEnvelope> apply(final JsonEnvelope event); | ||
default Stream<JsonEnvelope> apply(final JsonEnvelope event){ |
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.
update javadoc
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.
Done
final UUID clonedStreamId = eventSource.cloneStream(streamId); | ||
switch (requiresTransformation(eventStream, streamId)) { | ||
case TRANSFORM_EVENT: | ||
return transformEvent(streamId, eventStream); |
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.
this should be transformStream not events?
This language is a little bit confusing because the way the requresTransformation returns a result for a stream but has an action based on an Event.
Maybe if we just name the action TRANSFORM ?
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.
done
@@ -104,15 +129,39 @@ private JsonEnvelope clearEventVersion(final JsonEnvelope event) { | |||
).flatMap(identity()); | |||
} | |||
|
|||
private boolean requiresTransformation(final Stream<JsonEnvelope> eventStream) { | |||
return eventStream.filter(this::checkTransformations).count() > 0; | |||
private TransformAction requiresTransformation(final Stream<JsonEnvelope> eventStream, UUID streamId) { |
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.
final params
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.
Done
} | ||
|
||
private UUID archiveStream(final UUID streamId, final Stream<JsonEnvelope> eventStream) throws EventStreamException { | ||
eventStreamJdbcRepository.markActive(streamId,false); |
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.
formatting. space after comma
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.
Done
stream.append(transformedEventStream.map(this::clearEventVersion)); | ||
events.close(); | ||
} catch (Exception e) { | ||
logger.error("Failed to clone stream", e); |
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.
we need to say which stream id failed to clone
Is the only failure that can occur a clone? isn't the clone action just a small part of this?
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.
changed the logger statement to accept the stream Id parameter.
Not quite sure about the exception as this is an existing implementation and not changed as part of this work.
.distinct() | ||
.collect(Collectors.toList()); | ||
|
||
if (eventTransformationList.size() == 0) { |
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 use isEmpty?
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.
Done
return noAction(eventStream, streamId, "Stream {} did not require transformation stream "); | ||
} | ||
if (eventTransformationList.size() > 1) { | ||
return noAction(eventStream, streamId, "Stream {} can not have multiple actions "); |
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.
would it be useful to log/return what the multiple actions were?
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.
Done. added in logger the multiple action names
|
||
} | ||
|
||
private TransformAction noAction(Stream<JsonEnvelope> eventStream, UUID streamId, String errorMessage) { |
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.
final params
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.
Done
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 think the general structure looks ok, just a few minor comments.
Not 100% sure if there is a better way to do the switch statement to make it open for extension, but is not to fix now.
No description provided.