Remove an enrichment information from events before storing to AggregateStorage
#602
Conversation
Codecov Report
@@ Coverage Diff @@
## master #602 +/- ##
============================================
+ Coverage 92.83% 92.86% +0.02%
- Complexity 2730 2732 +2
============================================
Files 329 330 +1
Lines 9377 9387 +10
Branches 625 625
============================================
+ Hits 8705 8717 +12
+ Misses 490 489 -1
+ Partials 182 181 -1 |
|
@armiol PTAL. |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmytro-grankin please see my comments. Also, as discussed, please mention that enrichments are only cleared from the first level of origins.
| case ORIGIN_NOT_SET: | ||
| // Does nothing because there is no origin for this event. | ||
| break; | ||
| default: |
There was a problem hiding this comment.
As long as this method is public this branch can be tested. Please do.
There was a problem hiding this comment.
It cannot be tested even with using of reflection.
| switch (originCase) { | ||
| case EVENT_CONTEXT: | ||
| resultContext.setEventContext(context.getEventContext() | ||
| .toBuilder() |
There was a problem hiding this comment.
As discussed, do not clear this. And please mention that in docs.
| public static Enrichment enabledEnrichment() { | ||
| final String key = newUuid(); | ||
| final Any value = pack(toMessage(newUuid())); | ||
| return Enrichment.newBuilder() |
There was a problem hiding this comment.
Please have a result for such a complex expression before returning it.
|
@armiol PTAL. |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmytro-grankin LGTM for the most part. See the comments.
|
|
||
| /** | ||
| * Creates a non-{@linkplain io.spine.validate.Validate#isDefault(com.google.protobuf.Message) | ||
| * default} {@link Enrichment}. |
There was a problem hiding this comment.
Please describe what's in there. And then you'll be able to remove this "non-default" phrase.
| import static io.spine.protobuf.TypeConverter.toMessage; | ||
|
|
||
| /** | ||
| * Factory methods to create {@code Enrichment} instances for test purposes. |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmytro-grankin please also see one more comment.
| return context.getExternal(); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Let's discuss this description. Right now the name of the method does not correspond with it.
In particular, the description says that this is a procedure one would do before storing an event. And the rest of the content is about enrichments. So to me it looks more like a clearEnrichments() method. And even so I would adjust a few things.
However, we can stay with compact(). I like the name and the idea behind this name. However, there should be a more generic description, that this one.
Let's discuss this change.
|
@armiol PTAL. |
|
@armiol PTAL. |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmytro-grankin LGTM with the exception of two comments to address before merge.
| default: | ||
| throw newIllegalStateException("Unsupported origin case is encountered: %s", | ||
| originCase); | ||
| } |
There was a problem hiding this comment.
Please extract a result variable.
| public class GivenEnrichment { | ||
|
|
||
| private GivenEnrichment() { | ||
| // Prevent instantiation of this utility class. |
There was a problem hiding this comment.
Please reformat in a way Events utility does that.
Storage of
Aggregateevent records stores event enrichments, that aren't needed and take a lot of space.To fix this behavior,
AggregateStorage.writeEvent(...)method was changed and now it clears enrichments from the event context and the enrichment from the first-level origin of the specified event. But the event will still contain enrichments from second-level and deeper origins, because their removal is a heavy performance operation.Other changes:
EEntitywas fixed. Now, it removes only enrichments from an event.GivenEnrichmentutility was introduced.0.9.78-SNAPSHOT.