Conversation
hindessm
force-pushed
the
flexible-experiment
branch
2 times, most recently
from
b1d7a26 to
0cd800c
Compare
puellanivis
left a comment
puellanivis
left a comment
There was a problem hiding this comment.
Yeah, I was thinking of something like this when I was reading the other review.
I think it would improve the quality of the code in general.
| return re.registry | ||
| } | ||
|
|
||
| func (re *realEncoder) setFlexible(v bool) { |
There was a problem hiding this comment.
I’m not sure how much we need to be using setters, when we are our own package. We can reach into the struct and just set the value directly, no need to have a setter?
Oh… because it’s all accessed through an interface. 😐
hindessm
force-pushed
the
flexible-experiment
branch
from
0cd800c to
4d15744
Compare
|
🤔 Now that I think about it… we’re using an interface for encoders already right? … Why not provide two implementations one that uses varints, and one that doesn’t, and then just get a flexible or not direct implementation, and save execution from a bunch of if blocks. (Though, they’ll likely end up getting correctly predicted a lot in general.) |
|
I wondered about this too. If we did that, the caller would be responsible for creating the correct decoder implementation? And, If so, we'd need an In the encoder case, we'd need four encoders to handle the prep and real cases for flexible and non flexible. And again the caller would be responsible for picking the correct ones. |
|
We could just as easily combine the process into a And indeed. We trade the if-complexity for space-complexity. This is what the brainstorming draft is about, right? Figure out where we might want to put this. I think we could also however clean up a lot of the encoders and decoders, so long as we never need to use both the fixed, and compact encodings at the same time? |
Absolutely. (And I really appreciate your feedback!) There is inherent complexity in this area; we just need to figure out how to hide it so it isn't a distraction most of the time. The initial refactoring removed 330+ lines and made the message code more readable which in itself validates that trying to do something to hide the complexity is worth doing. I'm distracted by something else right now - CreateTopics V5 needs tagged fields so I'm prototyping handling them - but I'll try the distinct flexible/non-flexible encoder/decoder approach out to see how it is to use (possibly on another branch like this one). |
hindessm
force-pushed
the
flexible-experiment
branch
3 times, most recently
from
4dbea31 to
2a7e87f
Compare
|
I've created a branch with a very rough first attempt at splitting the flexible and non-flexible decoding. (The encoding will be trickier as we need to split the prep and real encoders.) |
Handle flexible encoding in more transparently to avoid if/else pattern making code harder to read/maintain. Basic pattern for each protocol message is: * if version 0 and greater is flexible just call the `Compact` methods directly, otherwise: * call `pe.setFlexible(r.Version >= <first-flexible-version>)` at beginning of `encode` method, and use the non-`Compact` methods and `maybePutEmptyTaggedFields()` as required. Signed-off-by: beanz <[email protected]>
Handle flexible decoding in more transparently to avoid if/else pattern making code harder to read/maintain. Basic pattern for each protocol message is: * if version 0 and greater is flexible just call the `Compact` methods directly, otherwise: * call `pd.setFlexible(version >= <first-flexible-version>)` at beginning of `decode` method, and use the non-`Compact` methods and `maybeGetEmptyTaggedFields()` as required. Signed-off-by: beanz <[email protected]>
Also moved version >= 5 check inside version >= 4 block to avoid redundant check in v <= 3 case. Signed-off-by: beanz <[email protected]>
Version 1 is the first flexible version Signed-off-by: beanz <[email protected]>
Version 2 is the first flexible version Signed-off-by: beanz <[email protected]>
Version 4 is the first flexible version Signed-off-by: beanz <[email protected]>
Includes tagged field handling V5 is first flexible version V5 also returns topic configs in the response (KIP-525) Signed-off-by: beanz <[email protected]>
hindessm
force-pushed
the
flexible-experiment
branch
from
2a7e87f to
629bd3f
Compare
|
I've updated my branch with the encoding side. |
|
@puellanivis I've done a bit of clean up of my other branch that took the approach you suggested (assuming I understood you correctly). I'm actually now pretty happy with that approach. |
|
Yes between these two I definitely think I prefer the other branch – it has turned out quite well |
|
Closing as superseded by (merged) #3317 |
|
After looking (late) through many of the new code, it looks good whatever decision you guys made. 😂 |
|
@puellanivis Assuming I understood your comment correctly, then we ended up adopting the approach you proposed here. I've added quite a few more flexible message versions since and haven't regretted the approach we've taken as it makes the code much easier to read and the updates required are as small as I'd hoped. Thanks very much for your input on this topic. |
3 participants
I plan to update all the protocol support to v2.4 level. While investigating what was involved, I started experimenting with slightly different approach to dealing with flexible versions (as several of the required protocol updates are just flexible version updates).
Before I go to far down this path, I'd be interested in any feedback so I thought I'd open a wip PR. (I've included a couple of protocol updates just to exercise the new mechanism and get a feel for writing with it but I'll probably break up the eventual non-draft PR.)