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.

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

Merged
merged 27 commits into from
Mar 15, 2015
Merged

Stateless broker #1935

merged 27 commits into from
Mar 15, 2015

Conversation

benbjohnson
Copy link
Contributor

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.

b.done = make(chan struct{})
go b.continuousQueryLoop(b.done)
// FIX(benbjohnson)
// b.done = make(chan struct{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are CQs disabled?

Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@otoolep
Copy link
Contributor

otoolep commented Mar 13, 2015

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.

@beckettsean beckettsean added this to the Next Release milestone Mar 14, 2015
@toddboom toddboom modified the milestones: Next Point Release, 0.9.0 Mar 14, 2015
// 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
Copy link
Member

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.

Copy link
Contributor Author

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.

@otoolep
Copy link
Contributor

otoolep commented Mar 14, 2015

+1

@toddboom
Copy link
Contributor

Unless anyone has a strong opinion otherwise, I'll merge this in first thing tomorrow and cut RC12.

toddboom added a commit that referenced this pull request Mar 15, 2015
@toddboom toddboom merged commit 236ab6f into master Mar 15, 2015
@toddboom toddboom deleted the stateless-broker branch March 15, 2015 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants