-
Notifications
You must be signed in to change notification settings - Fork 262
pkg/types/path Distinguish between /topicA and /topicA/ #430
Conversation
Signed-off-by: Yuji Oshima <[email protected]>
Signed-off-by: Yuji Oshima <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #430 +/- ##
==========================================
- Coverage 62.49% 62.42% -0.07%
==========================================
Files 73 73
Lines 4495 4506 +11
==========================================
+ Hits 2809 2813 +4
- Misses 1356 1359 +3
- Partials 330 334 +4
Continue to review full report at Codecov.
|
pkg/broker/server/sse.go
Outdated
for ch := range chset { | ||
for ch, value := range chset { | ||
if value.exactMatch && event.topic != key { | ||
fmt.Printf("skip notifier event.topic%v, key %v\n", event.topic, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this?
pkg/broker/server/sse.go
Outdated
fmt.Printf("skip notifier event.topic%v, key %v\n", event.topic, key) | ||
return false | ||
} | ||
fmt.Printf("notifier event.topic%v, key %v\n", event.topic, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Can you remove this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry! I removed.
pkg/broker/server/sse.go
Outdated
@@ -80,6 +87,13 @@ func clean(topic string) string { | |||
return topic | |||
} | |||
|
|||
func checkExactMatch(topic string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be written as one line right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! fixed.
pkg/broker/server/sse.go
Outdated
@@ -167,7 +181,7 @@ func (b *Broker) run() { | |||
// Disconnect all clients | |||
b.clients.Walk( | |||
func(key string, value interface{}) bool { | |||
chset, ok := value.(map[chan []byte]int) | |||
chset, ok := value.(map[chan []byte]clientValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we just have a simpler value in the radix tree without the type clientValue
?
We really just need something like map[chan[]byte]bool
, where the value bool is exactMatch or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this because I was not sure how to use the original int
. Fixed, thanks!
pkg/broker/server/sse.go
Outdated
} | ||
|
||
type event struct { | ||
topic string | ||
data []byte | ||
} | ||
|
||
type clientValue struct { | ||
value int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need this struct type at all. We only need to store the value in the radix tree as map[chan []byte]bool
where bool is just the exactMatch
value. We don't need this value
field...
Signed-off-by: Yuji Oshima <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!!
Display NAT rules for SSH LB in output for ARM
@chungers In #429, behavior seems to be different from what we were discussing.
It can not distinguish between topic /A and topic /A/.
Therefore, subscribing
/toppicA
will always subscribe all of/topicA/
and below subtopic, you can not subscribe only/topicA
.I added test
TestBrokerSubscriberExactMatchTopic
. This is the test I want to do in this PR.Signed-off-by: Yuji Oshima [email protected]