-
Notifications
You must be signed in to change notification settings - Fork 12
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
System feature configuration #1115
Conversation
# Conflicts: # license-report.md
Codecov Report
@@ Coverage Diff @@
## master #1115 +/- ##
===========================================
- Coverage 92.41% 92.3% -0.11%
- Complexity 3951 3971 +20
===========================================
Files 526 529 +3
Lines 12694 12747 +53
Branches 721 722 +1
===========================================
+ Hits 11731 11766 +35
- Misses 745 762 +17
- Partials 218 219 +1 |
Codecov Report
@@ Coverage Diff @@
## master #1115 +/- ##
============================================
- Coverage 92.46% 92.38% -0.08%
- Complexity 3960 3996 +36
============================================
Files 527 530 +3
Lines 12719 12786 +67
Branches 725 729 +4
============================================
+ Hits 11760 11812 +52
- Misses 742 757 +15
Partials 217 217 |
@armiol, PTAL. |
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.
@dmdashenkov please see my comments.
import io.spine.annotation.Internal; | ||
|
||
/** | ||
* System bounded context feature configuration. |
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.
Please rephrase so that the relations are clear.
} | ||
|
||
/** | ||
* Obtains the Command log setting. |
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.
Let's have the spelling of Command log
(@link CommandLog
above, etc) consistent.
* | ||
* <p>By default, the system context: | ||
* <ol> | ||
* <li>Stores Aggregate mirrors for querying. |
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.
Let's speak of this as of Enables querying of the latest {@code Aggregate} states.
And I would name the aggregateMirrors
field in the same manner.
@armiol, PTAL. |
@alexander-yevsyukov, PTAL. |
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.
Please see my comments.
|
||
private ContextSpec(BoundedContextName name, boolean multitenant) { | ||
private ContextSpec(BoundedContextName name, boolean multitenant, boolean storeEvents) { |
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.
The storeEvents
parameter looks weird and not native for a regular BoundedContext
. What a context does not store events? It can be equally named isSystem
... :) This looks like the idea of context hierarchy and its properties leaked into here.
*/ | ||
@Override | ||
public EventStore createEventStore(ContextSpec context) { | ||
return context.storesEvents() |
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.
Once again, there is a conflict between the name SystemAware...
and non-system-awareness in the storesEvents()
. Why not just have isSystem()
then?
public boolean includePersistentEvents() { | ||
return storeEvents; | ||
} | ||
} |
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.
equals()
, hashCode()
, toString()
?
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.
On the other note, if we were building a server environment in another language, could it be a Message
-based type? Don't we describe a System
context after all?
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.
Assuming discussed vocally arguments and confirmed changes, LGTM.
This PR introduces API for configuring System context features.
Previously, all the capabilities of the System context used to be mandatory for all users.
While all of the features of the System context may be useful to some users, other users may live completely without them. That said, users should be cautious to turn off system features, as they might still need the information provided by a given system capability later.
Users may configure:
CommandLog
for each domain command (OFF by default);System events might be required later to catch up domain projections (for instance, if they rely on entity state updates).
Command logs might be required for audits or debugging. This is the only place in the system where the actual commands, and not their outcome, are stored.
Mirrors allow querying domain aggregates as efficiently as they were projections, i.e. without replaying recent events. Without mirrors, domain aggregates cannot be queried via
QueryService
.The reason to turn these features off is to improve the performance of the configured
Delivery
and to reduce the amount of stored data.