Skip to content
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.

Sign up for GitHub

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

Fix nsq_consumer regression by avoiding connection to an empty list of servers #9503

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Fix nsq_consumer regression by avoiding connection to an empty list of servers #9503

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions plugins/inputs/nsq_consumer/nsq_consumer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package nsq_consumer

import (
"context"
"fmt"
"sync"

"github.com/influxdata/telegraf"
Expand Down Expand Up @@ -134,15 +135,28 @@ func (n *NSQConsumer) Start(ac telegraf.Accumulator) error {
return nil
}))

// For backward compatibility
if n.Server != "" {
n.Nsqd = append(n.Nsqd, n.Server)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation says we prepend it to the list, but we're appending it here. Could be a concern if the server connection list order matters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all thanks a lot for the review!
Regarding the order, I just used what was done before in this plugin (previous line 143). I can change this, should I?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably change the docs to say append. If the plugin appended before, changing it to prepending might cause incompatibility.

}

// Check if we have anything to connect to
if len(n.Nsqlookupd) == 0 && len(n.Nsqd) == 0 {
return fmt.Errorf("either 'nsqd' or 'nsqlookupd' needs to be specified")
}
Comment on lines +144 to +146
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about moving the server len checks to an func (n *NSQConsumer) Init() error function? This way it can error on startup that the configuration is invalid, rather than at connect time.
That might also help with another potential issue; if the Start function is called more than once (say connection failure), the above code will keep appending to the n.Nsqd slice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your point is perfectly valid and makes sense to me. I tried to touch as little as possible as this is a regression fix. Do you want me to fix this in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge the regression fix as-is so it's in 1.19.1 tomorrow and make another pr to implement Steven's recommendation.


if len(n.Nsqlookupd) > 0 {
err := n.consumer.ConnectToNSQLookupds(n.Nsqlookupd)
if err != nil && err != nsq.ErrAlreadyConnected {
return err
}
}
err := n.consumer.ConnectToNSQDs(append(n.Nsqd, n.Server))
if err != nil && err != nsq.ErrAlreadyConnected {
return err

if len(n.Nsqd) > 0 {
err := n.consumer.ConnectToNSQDs(n.Nsqd)
if err != nil && err != nsq.ErrAlreadyConnected {
return err
}
}

n.wg.Add(1)
Expand Down