Add the first draft of the RFC for the event notification improvement. #1066
Conversation
|
|
||
| However, if we already think about the first approach, we might as well | ||
| reconsider the event command argument of the notification, and expose it | ||
| too; just as well as we could allow the user's key space trigger to |
There was a problem hiding this comment.
Do we know about real world use cases that will benefit from exposing the command name?
How do we expect to handle renamed commands here?
There was a problem hiding this comment.
Unfortunately, I know nothing about the real-world use cases. @MeirShpilraien @thomascaudron Do you know, perhaps?
Handling renamed commands should be done by the user in this case, not by our side: we simply pass through the event command name string to the user.
There was a problem hiding this comment.
I think there is a confusion. Its not really the command name, its the event name that was fire by Redis (which IIRC is not effected by command renaming).
I do not know about usecases but I believe its an important information to expose (we already expose it today).
| In this case, however, there is again, might be a possible overlap of | ||
| the values of both, that needs to be checked at deploy time. | ||
|
|
||
| ## Option 4 (Listen to the "NEW" flag by default) |
There was a problem hiding this comment.
Other than being a breaking change, did you consider the other pros and cons of option 4 vs the others? I guess having a single function to handle all cases can be an advantage. But could that lead to performance issues, if, for example, the function doesn't care about some events?
There was a problem hiding this comment.
So right now, we don't have any filtering w.r.t. the flags, and we only filter by key name or key prefix while iterating over an array of user callbacks. There is no order within this array, so nothing is done to make the search faster. If we go this way, perhaps it makes sense to store a hashmap where the keys would be compatible event flags/event command names, and the keys would be a list of these user callbacks so we can look up the needed callbacks faster rather than going through all the callbacks iteratively. Perhaps the same optimisation could be done for the key names/prefixes; I need to check.
To summarise and answer the last question, if a function doesn't care about some events, we still can have a good performance optimisation to look up such callbacks and fire them quickly. However, the implicated cost of the finding itself, navigating to the callbacks objects, firing one by one, all the indirections, will still (and always) cause higher latency, for there still some processing takes place, and it is not for free. So, having one function that catches all of those might be better in terms of performance/resource use.
What do you think? @MeirShpilraien
|
|
||
| ## Option 4 (Listen to the "NEW" flag by default) | ||
|
|
||
| This is a **NOT** backwards-compatible way, which implies that all the |
There was a problem hiding this comment.
Maybe we can use the API version to avoid backward-compatibility issue?
There was a problem hiding this comment.
Looking at all the option I now realize that maybe it was wrong to mix the NEW support with the filtering support. I think that maybe, in order to support the NEW event, we can simply go with option 4 (and we can avoid backward compatibility by using the API version). And also we can discuss separately about the filter API.
I think this separation makes more sense. Its also feels weird that by default we pass all the even except for the NEW, and if the user wants to NEW event he needs to specify it on the filter. I think that default should be all the events, and filter should filter events out (not expose them).
No description provided.