Subscriber Billed Topics #609
Conversation
| log.Tag(tagSubscribe).Field("topic", t.ID).Debug("Canceling subscriber %s", s.userID) | ||
| if s.visitor.MaybeUserID() != exceptUserID { | ||
| // TODO: Shouldn't this log the IP for anonymous visitors? It was s.userID before my change. | ||
| log.Tag(tagSubscribe).Field("topic", t.ID).Debug("Canceling subscriber %s", s.visitor.MaybeUserID()) |
There was a problem hiding this comment.
Should that log be visitorID instead of userID, to also log anonymous visitors properly?
|
I looked, I promise. I just cannot get my brain into the right headspace, which I need for this. I promise I will :-) |
| } | ||
| return t.lastVisitor | ||
|
|
||
| } |
There was a problem hiding this comment.
- Style: Don't use snake_case, use camelCase. And: don't use getters (no getX), use
Xinstead, here:func billee() - This function should use the mutex!
- This is a strange way of retrieving the billee, IMHO. It took me very very very long to understand, which is probably not a good sign. So maybe we can do something where we assign it in
Subscribeand pick a new one inUnsubscribe(if we're removing the thetopicSubscriberwith the same visitor as intopic.lastVisitor
Something like this:
type topic struct {
vrate *visitor
vrateExpires time.Time(like you have, but rename it)
And then just assign it when you first subscribe:
if t.vrate = nil {
t.vrate = v
t.vrateExpires = ...
}
t.subscribers[subscriberID] = &topicSubscriber{
visitor: v,
..
}
There was a problem hiding this comment.
Updated my comment from this afternoon.
There was a problem hiding this comment.
1 and 2 are done, and I think the latest rewrite achieves 3?
| if v == nil { | ||
| return nil, errHTTPWontStoreMessage | ||
| } | ||
| } |
There was a problem hiding this comment.
I know I said to just replace v in an older comment, but the more I think about it, the more I think that this is literally just about rate limiting, right? So maybe maybe our Billee is just a vrate (= visitor used for rate limiting), and for all the rate limiting functions, we'd use vrate, but for everything else, we'd use v?
I have also noticed that you haven't tackled the limitRequests middleware at all. The rate limiting for number of messages (visitor-message-daily-limit) and number of emails (visitor-email-limit-*) happens in handlePublish*, but the rate limiting for requests (visitor-request-limit-*) happens in limitRequests, so this logic would have to happen there, not here.
There was a problem hiding this comment.
I did this using context (as suggested below), but isn't that supposed to be an anti-pattern? In any case, I guess it's just one thing so it shouldn't matter much. I feel like the ideal solution to this would be making visitor an interface and having a compositeVisitor in addition to visitor...but, that's probably overengineering.
| } | ||
| return t.lastVisitor | ||
|
|
||
| } |
There was a problem hiding this comment.
Updated my comment from this afternoon.
|
Cool. I'll start reviewing again, but you gotta repoint to |
|
I'm working on the rest of the things
…On Mon, Feb 20, 2023, 6:46 PM Philipp C. Heckel ***@***.***> wrote:
Cool. I'll start reviewing again, but you gotta repoint to main and
rebase/merge the latest main.
—
Reply to this email directly, view it on GitHub
<#609 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHZIPSQDN3HITZ6WCGWE7ODWYQF6FANCNFSM6AAAAAAU377XDE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Superseded by #633 |
I am going with bill-to-last-visitor-for-12-hrs even though it doesn't allow users to unsubscribe because it is the simplest to explain. It's much easier to document and explain if you subscribe to a
up_topic, you are responsible for whatever is sent there for the next 12 hrs, so keep it secret, than anything else I could come up with.The only other simple-for-the-user options would be:
sub := func(v *visitor, msg *message) error {, so only messages you actually receive count against you. However, that allows spamming the cache with messages that no one is billed for, on the sending side.I'll add tests and rebase it and stuff if you're good with this general implementation.