-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Stateless broker #1935
Stateless broker #1935
Conversation
…oker-truncation
Conflicts: messaging/broker.go
…ateless-broker Conflicts: cmd/influxd/run.go messaging/broker.go messaging/client.go messaging/client_test.go messaging/intg_test.go server.go tx_test.go
…ateless-broker
…ateless-broker
b.done = make(chan struct{}) | ||
go b.continuousQueryLoop(b.done) | ||
// FIX(benbjohnson) | ||
// b.done = make(chan struct{}) |
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.
Why are CQs disabled?
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 remember Ben saying something about the brokers not knowing about the data nodes in this new branch, but I think that they should. The data nodes have to connect to get replication so they should include a connect string so that the raft leader can send requests down to the data nodes.
The brokers will need to know about the data nodes anyway so that they can redirect a data node that requests a truncated topic to another data node that can give them a copy of the entire thing.
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.
The brokers now only track the highest index received by any data node so we aren't tracking the actual data node URLs themselves. I realized that was an issue once I got to CQs and remembered that they need cluster info to work.
I talked to Paul about this and we decided to send the data node URLs up to the broker. There's a periodic ping that occurs from the client so I can add it there. I disabled the CQs temporarily because it was more important to get messaging stable.
path: t.path, | ||
}) | ||
} | ||
// Read segments from disk, not topic. |
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.
Minor: I don't get this comment. Where else could segments come from? They are on the disk, and make up topics. I don't think there is any confusion, or am I missing something?
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.
Fixed. That comment was left over from when the topic managed the segments.
Big change obviously, I took a first pass, and have some questions. I generally understand what is going on. i do like the idea of a each shard having its own connection, and writing data to disk itself. It will be interesting to see this in practise. I'm up for further review, once the build is green. |
// DefaultPollInterval is the default amount of time a topic reader will wait | ||
// between checks for new segments or new data on an existing segment. This | ||
// only occurs when the reader is at the end of all the data. | ||
const DefaultPollInterval = 100 * time.Millisecond |
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.
What happens if this gets bumped down to something like 10ms? I'm just wondering because it seems like most of the time topic readers will be at the end of their topic. So they'll poll, read whatever came in during that time. Then wait, then poll again.
Just wondering if it's ok to have this be low.
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 is a stop gap solution. A better solution is to only allow streaming topic readers to be created from topics and then have the topics notify the readers when there's an update to a topic through a channel. That would get rid of a ton of overhead in reading the directory, creating segments, matching segments, etc.
I can add that in a separate PR.
… into stateless-broker
+1 |
Unless anyone has a strong opinion otherwise, I'll merge this in first thing tomorrow and cut RC12. |
Overview
This pull request removes replicas and subscriptions from the brokers and makes much more simplistic. It also removes cluster-wide sequential reads and instead uses per-topic sequential reads. There is still a global unique index though.