-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
support sampling time to ack messages in explicit ack mode #1225
Conversation
|
45d0dea
to
9161a28
Compare
server/msgset.go
Outdated
@@ -113,7 +113,7 @@ func (a *Account) AddMsgSet(config *MsgSetConfig) (*MsgSet, error) { | |||
} | |||
// Setup the internal client. | |||
c := s.createInternalJetStreamClient() | |||
mset := &MsgSet{jsa: jsa, config: cfg, client: c, obs: make(map[string]*Observable)} | |||
mset := &MsgSet{jsa: jsa, config: cfg, client: c, obs: make(map[string]*Observable), mu: sync.RWMutex{}} |
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.
seems unlikely that this would be needed but I triggered this without it
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1439c7e]
goroutine 934 [running]:
github.com/nats-io/nats-server/v2/server.(*MsgSet).Name(0x0, 0x0, 0x0)
/Users/rip/go/src/github.com/nats-io/nats-server/server/msgset.go:432 +0x2e
github.com/nats-io/nats-server/v2/server.(*Observable).sampleAck(0xc0000ab6c0, 0x1, 0x1)
/Users/rip/go/src/github.com/nats-io/nats-server/server/observable.go:540 +0x68
github.com/nats-io/nats-server/v2/server.(*Observable).ackMsg(0xc0000ab6c0, 0x1, 0x1)
/Users/rip/go/src/github.com/nats-io/nats-server/server/observable.go:561 +0x1a3
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.
Yea we should not need it, will fix elsewhere.
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.
msgset is nil.
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.
ah thats true, you're right it is the msgset, that really shouldnt happen :)
24b0cd3
to
a713543
Compare
after latest commits we have:
|
1702b1f
to
b9dcdb7
Compare
I think we could be more consistent with naming. Some ideas below.
|
This allows an observable to have a sampling frequency and will publish ack times regularly
c6efaaf
to
2ef4677
Compare
updated key names and rebased onto latest jetstream branch |
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.
LGTM
A quick spike on the idea I mentioned of sampling ack times in observables