Skip to content
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

Serde options refactor #506

Closed
wants to merge 13 commits into from
Closed

Conversation

lovromazgon
Copy link
Contributor

This is a follow-up to #467 (comment).

This PR includes the following changes:

  • Rename Opt to ClientOpt
  • Rename SerdeOpt to EncodingOpt
  • Introduce a new SerdeOpt, which will ultimately be used to set the SerdeHeader (this can only be included once Serde Header #467 is merged)
  • Introduce the encoding option ID which lets you specify which schema ID to use when encoding (specifically useful when encoding dynamic data)

pkg/sr/serde.go Outdated
Comment on lines 426 to 427
// Load tserde based on the supplied ID.
t = s.loadIDs()[int(idopt.ID())]
Copy link
Contributor Author

@lovromazgon lovromazgon Jul 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last commit adds the ability to supply EncodingOpt to Decode, simply to have parity with Encode, but I'm not convinced if it makes sense. I included it to run it by you, but I can revert the change if needed.

This is how it works. If you supply options ID and Index to Decode it will find tserde based solely on the supplied options, it will not decode the header. In fact, it will assume there is no header and that it's dealing only with the raw encoded bytes. What I don't like about this is that Serde can't encode bytes without a header unless you specify your own SerdeHeader implementation which skips prepending the header. It would allow you to track the ID and index separately from the encoded payload, although I'm not sure such a use case exists, so it might needlessly complicate the API.

@lovromazgon lovromazgon mentioned this pull request Jul 9, 2023
pkg/sr/serde.go Outdated
t, ok := s.loadTypes()[reflect.TypeOf(v)]
if !ok || (t.encode == nil && t.appendEncode == nil) {
return b, ErrNotRegistered
func (s *Serde) AppendEncode(b []byte, v any, opts ...EncodingOpt) ([]byte, error) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of having options at the point of encoding?
Same thought on decoding.
I'd like to avoid options on these functions if possible -- would it be possible to have a block of Dyn(Append)?Encode functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @twmb! First of all, sorry, this work fell through the cracks. I'll try to find some time over the weekend and clean up the code. IIUC you're proposing to have DynEncode and DynAppendEncode that take EncodingOpt and leave the existing encode functions as they are, correct? I'll try it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As promised, I added the changes. It simplified the code quite a bit - no more ID option, no more special treatment of any option, no more options passed to encode and decode. Instead we now have DynEncode and related functions, which expect parameters id and index to determine the correct tserde.

I'm looking forward to hear your feedback 😉

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random question now that I'm seeing this PR:

Is there any reason to require registering the types at all and then using DynEncode, vs. having top-level, non Serde method functions,

func Encode(id int, index []int, v any, enc func(any) ([]byte, error)) ([]byte, error)
func AppendEncode(b []byte, id int, index []int, v any, enc (func([]byte, any) ([]byte, error)) ([]byte, error)

?

I'm wondering: what use case do you have that allows you to know the id and index you want to encode, but not know the encode function?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just as a heads up -- I have a big sr update coming in #548, which should also move the needle on package stabilization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh well, this was another long delay, sorry about that. You know, it's the standard apology, being busy. I'll try to be more responsive now and help you resolve this PR.

I merged the changes from master and resolved the conflicts introduced in the PR you mentioned.

what use case do you have that allows you to know the id and index you want to encode, but not know the encode function?

It's not that I don't know the encode function, I do know it actually. It's that Serde also appends the header and I don't want to duplicate that logic outside of Serde. Another reason to register the type is that the next time the type gets decoded in a different path of the code, we already have a Serde that knows how to decode it, no need to fetch the schema.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries on the delay, I'm just as slow in the past few months due to games and traveling and holidays and work :)

I'm not sure I still understand but I'm sure I would if I see what you're trying to do -- do you have an example of how you intend to use the dyn functions that can't be served with the current API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the line where we call Serde.Encode and provide an ID of the schema to use. (For completeness, here's Serde.Decode).

I re-read your suggestion a few comments above and I think that having top-level, non Serde functions for Encode and AppendEncode that let you specify the ID and the encode func could work. The calls wouldn't go through the Serde type, but I realized we don't reuse the same object in the decode and encode paths of the code, so no harm done. And the solution feels cleaner than having DynEncode.

I'll see if I can get some time in the next few weeks to work this into the PR.

@twmb twmb mentioned this pull request Mar 26, 2024
12 tasks
@twmb twmb added the TODO label May 23, 2024
appendEncode := t.appendEncode
if appendEncode == nil {
// Fallback to t.encode.
appendEncode = func(b []byte, v any) ([]byte, error) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this causes an extra alloc in the encode path.

Copy link
Owner

@twmb twmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm +1 on the changes now, minus the one comment about AppendEncode. I'll see about rebasing your branch on the latest release and see about getting this merged.


b, err := s.header().AppendEncode(b, int(t.id), t.index)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was lost (and not added in one of the new top level functions)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, it's used in the new call to AppendEncode below.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might rather have a little bit of duplication, not sure yet.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep leaning duplication; I rebased on master and squashed all of your commits and did a few touchups in #742.

twmb pushed a commit that referenced this pull request May 26, 2024
* Renames Opt to ClientOpt
* Renames SerdeOpt to EncodingOpt
* Adds SerdeOpt, with the ability to set the header that is always used
* Adds top-level Encode/AppendEncode functions, to be used when you are
  dynamically encoding types and their ids / indices

This is #506 by @lovromazgon, rebased on master, squashed into one
commit, with minor modifications
@twmb twmb mentioned this pull request May 26, 2024
twmb pushed a commit that referenced this pull request May 26, 2024
* Renames Opt to ClientOpt
* Renames SerdeOpt to EncodingOpt
* Adds SerdeOpt, with the ability to set the header that is always used
* Removes Serde.SetDefaults, in favor of the new (and more standard)
  NewSerde function
* Adds top-level Encode/AppendEncode functions, to be used when you are
  dynamically encoding types and their ids / indices

This is #506 by @lovromazgon, rebased on master, squashed into one
commit, with minor modifications
@twmb
Copy link
Owner

twmb commented May 26, 2024

Merged in #742, ty for the work (and patience over time 😅 😬)

I think the main thing I want to think about before stabilizing the sr package is whether the top level Encode and AppendEncode functions should take all their args as fields, vs. some form that allows for future growth. I'm currently leaning to keep the current form (though maybe swap the h argument before id and index).

@twmb twmb closed this May 26, 2024
@twmb
Copy link
Owner

twmb commented May 26, 2024

@lovromazgon I'm swapping SerdeHeader before id,index in #743

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants