-
Notifications
You must be signed in to change notification settings - Fork 175
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
Per-topic MsgIdFunction #464
Comments
Is this actually necessary? |
Note that there is no inherent need for ugly switching, one can use a map with a readonable default. |
Imagine there are two completely different protocols relying on PubSub. One of them needs custom msg id generation and another one is not. The application which relies on these two protocols and creates a PubSub instance would need to be aware of the protocol internals and import their msg types to do the switch. So the ugliness comes not from the switch itself but from the ugly architecture requiring the application to know the internals. Sorry for the confusion. |
The point is to allow msg id generation to be application/protocol/layer specific. In my case, I need to exclude one non-deterministic field of a structure from id generation with blake2b, but I don't want to have one global |
Well ok, but it complicates things. |
How? |
The right way to do it internally is to have one msgid function, which dsipatches on a map of funcs set potentially per topic and a default that can be overwritten as is now with the option. |
this way the internal changes will be minimal, and we dont break existing applications. |
The solution I proposed keeps the logic and API the same and still allows customization. If In other words, |
That's not very elegant i think, it makes users reinvent the wheel and messy internals. Lets create a struct type that has a map and a default, with a RWMutex. This way nothing changes in the external interface and on the same time we get a maximally flexible solution. |
Btw, for future reference, it is preferrable to discuss design in an issue and reach consensus before opening the pr, so that you dont do unnecessary work. |
I am in 👍🏻
Thank you for the detailed description. Qs:
Agree and I wanted to write a proposed solution but it turned out to be easier to implement it instead of describing it in English for this case :)
|
Yeah, sometimes it is easier and that's totally fine, I just don't want you to do work that might be thrown away at design stage. Then again you learn the internals better by doing that, so it's not a total loss. |
Note that the cache is new field we add on the Message wrapper struct. |
Also note that we should relinquish the rlock before invoking the compute function, so that we don't block other stuff while it is computing. |
We still need to change
This is also why I originally thought of the map with |
Ah yes indeed, but thats a very small change. |
2022-01-07: there is a linked PR (#465 ) that we're trying to get across the line. |
Pubsub only allows setting global
MsgIdFunction
, which applies to all the topics. Still, It would be useful to have the ability to set custom per topic msg id generation, which, e.g., can be dependant on the msg format of some topic. For this to be implemented using the globalMsgIdFunction
, it would need to be aware of all the msg types of each topic and do some ugly switching to determine whatMsgIdFunction
to use for what msg.Fortunately, an unused
TopicOpt
can set custom values while joining the topic, so some per topic customizations are conceived by design, and the described feature can be implemented.The text was updated successfully, but these errors were encountered: