-
Notifications
You must be signed in to change notification settings - Fork 37
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
[DEVEX-222] - Auto serialization feedback improvements #338
base: DEVEX-185-Rebranding
Are you sure you want to change the base?
[DEVEX-222] - Auto serialization feedback improvements #338
Conversation
0f2f0be
to
23ed48e
Compare
e7adb5b
to
abccac0
Compare
82962d1
to
676cc36
Compare
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.
Looking good to me
@@ -195,7 +187,7 @@ IWriteResult HandleWrongExpectedRevision( | |||
actualStreamRevision | |||
); | |||
|
|||
if (operationOptions.ThrowOnAppendFailure) { | |||
if (operationOptions.ThrowOnAppendFailure == true) { |
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.
Minor, but had me go "why?". Did it become nullable perhaps?
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.
Yes, and no 🙂
Yes because ThrowOnAppendFailure
indeed is nullable now, as it can be provided or not by the user. I prefer to keep it nullable to have clear information whether the user sets it or not.
No because it's different OperationSettings than before. Previously we used here KurrentDBClientOperationOptions
, which are general settings for the client, and it contains various options that may not be related to specific operations. Previously they were passed even to read operations, where ThrowOnAppendFailure
wouldn't make any sense. I replaced them with more explicit and contextual AppendToStreamOptions
and generic OperationOptions
.
/// <param name="userCredentials">The optional <see cref="UserCredentials"/> to perform operation with.</param> | ||
/// <param name="cancellationToken">The optional <see cref="System.Threading.CancellationToken"/>.</param> | ||
/// <returns></returns> | ||
public static Task<StreamMetadataResult> GetStreamMetadataAsync( |
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.
Was the intent for these to carry an [<Obsolete(...)>] attribute?
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.
Yes, good call! I forgot to mark them as obsolete. Did that in 1a8e507.
9eefd3a
to
db4fc93
Compare
4e1cf75
to
7815a6f
Compare
47a3cff
to
1c489b8
Compare
95eb311
to
f690270
Compare
…ting in AppendToStreamAsync method
…stream Added also override for raw event append that has options and made other methods obsolete. Introduced OperationOptions that compose default client operation settings plus credentials and deadlines.
…on as regular system metadata
Added also factor for event data that's not requiring to provide id immediately
…r EventData, and better aligned with general usage of messages instead of just events
…s for read results That should be a middle ground without forcing people to use a regular ResolvedEvent if they don't want to
Now serializer creates a derived serializer merging operation settings with default settings if needed. That allowed to hide internal details and there's no need to pass content type explicitly, as it's already in settings. Extended SchemaRegistry to provide wrapper methods to naming resolution context. Thanks to that MessageSerializer doesn't need to reference it internally. To make responsibilities clearer made also naming resolution context part of serializer context.
…vided options or not
…, so e.g. FromEnd assigns also backwards if it wasn't set
f690270
to
6d48b9c
Compare
…tead of just raw string. That should enable current users to override their strategy if they used something more than just message type name from EventRecord. It needs to be EventRecord and not ResolvedEvent, as resolution happens before creating ResolvedEvent.
…ge type names mapping to the same CLR class
Tha makes naming strategy focused on just mapping types back and forth and allowing to disable auto registration
This PR follows up on the initial PR that introduced auto-serialization and applies the discussed changes.
Brought back expected stream state to append to stream methods. Even if users may decide to ignore optimistic concurrency check by using
StreamState.Any
and it may be a valid decision, we want to promote using optimistic concurrency and making it a cautious decision to resign from it.Introduced of
OperationOptions
, which wraps common settings like deadline and credentials. I used it as the base class for reading, appending, and internal implementations. That's also aligned with the current Java client approach. This can be also preparation for future changes like batch appends.Introduced
MessageData
and used it instead ofEventData
. As we want to promote KurrentDB as a general message store, and introducedMessage
type as a wrapper for the serialization then it's a logical step. Made EventData be possible to cast to message map to make easier migration and marked it as obsolete. Also applied some usability improvements in those classes (boy scout rule).Added raw append to stream with options to give people a clear path when they want to provide raw bytes. Thanks to that, other append methods with dozens of parameters were obsolete and moved to extension methods. This should be less confusing, as now we have two official paths: auto-serialization and raw appends. The rest are clearly stated as obsolete.
Added
DeserializedData
andDeserializedMessages
extension methods for read results. Thanks to that, people can easily map message data if they don't want to use the fullResolvedEvent
. That makes clearer usage path for folks that offload all serialization and message mapping to us. You can use it as:Besides that, simplified the
MessageSerializer
. Now it creates a derived serializer merging operation settings with default settings if needed. That allowed hiding internal details, and there's no need to pass content type explicitly, as it's already in settings.Extended
SchemaRegistry
will provide wrapper methods for naming the resolution context. Thanks to that MessageSerializer doesn't need to reference it internally.To make responsibilities clearer, made also naming resolution context part of the serializer context.
Aligned metadata serializer with the default, system one. Those settings will be different than the ones in regular data, but this will ensure that changes are backward compatible to tracing, etc, made by existing users.
TODO: