Lazily connect to brokers in the client #309
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eapache
force-pushed
the
lazy-broker-connections
branch
2 times, most recently
from
0b187cf to
6b7ee17
Compare
|
This looks innocent enough 👍 Should we do some manual testing? |
|
Ya, I'm really suspicious mostly because it looks so innocent. Throw whatever cases you can think of at it :) |
|
This code looks good, what do you guys think about genghis like stress testing tool for kafka, we could possibly just leverage genghis ? |
|
Kind of unrelated to this PR, but we need:
Let's discuss this offline. |
Instead of opening a connection to all brokers immediately upon receiving their information in metadata, wait until we are asked for them either via a call to `Leader` or a call to `any`.
eapache
force-pushed
the
lazy-broker-connections
branch
from
6b7ee17 to
533cce2
Compare
eapache
added a commit
that referenced
this pull request
Mar 12, 2015
It is now only called from one place in the client. This looks simple but is actually super-subtle, and depends on lazy broker connections (PR #309). disconnectBroker does a whole bunch of different things: - calls `Close` on the broker connection - adds the address to the internal `deadBrokerAddrs` map even if the broker is not a seed, which I think is wrong, since resurrectDeadBrokers will use it to repopulate the seedBrokerAddrs list - rotates seedBrokers (if the broker was a seed broker) - removes it from the brokers map (otherwise) In the producer and consumer where we used to call disconnectBroker: - We now call `Close` directly on the broker. - The broker we are dealing with is not a seed broker, so the seedBrokers do not need rotating, and I don't think it's a problem that it no longer gets added to `deadBrokerAddrs`. - The reason we removed it from the broker map was so that the next request for that broker would trigger a metadata request and reopen the connection. The producer and consumer both manually trigger metadata requests when necessary, and the fact that we now have lazy connection opening means simply closing it (which we do, see first bullet) is enough to cause the connection to reopen the next time it is requested, even if no metadata refresh is requested.
This was referenced Mar 12, 2015
eapache
added a commit
that referenced
this pull request
Mar 12, 2015
It is now only called from one place in the client. This looks simple but is actually super-subtle, and depends on lazy broker connections (PR #309). disconnectBroker does a whole bunch of different things: - calls `Close` on the broker connection - adds the address to the internal `deadBrokerAddrs` map even if the broker is not a seed, which I think is wrong, since resurrectDeadBrokers will use it to repopulate the seedBrokerAddrs list - rotates seedBrokers (if the broker was a seed broker) - removes it from the brokers map (otherwise) In the producer and consumer where we used to call disconnectBroker: - We now call `Close` directly on the broker. - The broker we are dealing with is not a seed broker, so the seedBrokers do not need rotating, and I don't think it's a problem that it no longer gets added to `deadBrokerAddrs`. - The reason we removed it from the broker map was so that the next request for that broker would trigger a metadata request and reopen the connection. The producer and consumer both manually trigger metadata requests when necessary, and the fact that we now have lazy connection opening means simply closing it (which we do, see first bullet) is enough to cause the connection to reopen the next time it is requested, even if no metadata refresh is requested.
eapache
added a commit
that referenced
this pull request
Mar 12, 2015
It is now only called from one place in the client. This looks simple but is actually super-subtle, and depends on lazy broker connections (PR #309). disconnectBroker does a whole bunch of different things: - calls `Close` on the broker connection - adds the address to the internal `deadBrokerAddrs` map even if the broker is not a seed, which I think is wrong, since resurrectDeadBrokers will use it to repopulate the seedBrokerAddrs list - rotates seedBrokers (if the broker was a seed broker) - removes it from the brokers map (otherwise) In the producer and consumer where we used to call disconnectBroker: - We now call `Close` directly on the broker. - The broker we are dealing with is not a seed broker, so the seedBrokers do not need rotating, and I don't think it's a problem that it no longer gets added to `deadBrokerAddrs`. - The reason we removed it from the broker map was so that the next request for that broker would trigger a metadata request and reopen the connection. The producer and consumer both manually trigger metadata requests when necessary, and the fact that we now have lazy connection opening means simply closing it (which we do, see first bullet) is enough to cause the connection to reopen the next time it is requested, even if no metadata refresh is requested.
eapache
added a commit
that referenced
this pull request
Mar 12, 2015
It is now only called from one place in the client. This looks simple but is actually super-subtle, and depends on lazy broker connections (PR #309). disconnectBroker does a whole bunch of different things: - calls `Close` on the broker connection - adds the address to the internal `deadBrokerAddrs` map even if the broker is not a seed, which I think is wrong, since resurrectDeadBrokers will use it to repopulate the seedBrokerAddrs list - rotates seedBrokers (if the broker was a seed broker) - removes it from the brokers map (otherwise) In the producer and consumer where we used to call disconnectBroker: - We now call `Close` directly on the broker. - The broker we are dealing with is not a seed broker, so the seedBrokers do not need rotating, and I don't think it's a problem that it no longer gets added to `deadBrokerAddrs`. - The reason we removed it from the broker map was so that the next request for that broker would trigger a metadata request and reopen the connection. The producer and consumer both manually trigger metadata requests when necessary, and the fact that we now have lazy connection opening means simply closing it (which we do, see first bullet) is enough to cause the connection to reopen the next time it is requested, even if no metadata refresh is requested.
eapache
added a commit
that referenced
this pull request
Mar 13, 2015
Lazily connect to brokers in the client
eapache
added a commit
that referenced
this pull request
Mar 13, 2015
It is now only called from one place in the client. This looks simple but is actually super-subtle, and depends on lazy broker connections (PR #309). disconnectBroker does a whole bunch of different things: - calls `Close` on the broker connection - adds the address to the internal `deadBrokerAddrs` map even if the broker is not a seed, which I think is wrong, since resurrectDeadBrokers will use it to repopulate the seedBrokerAddrs list - rotates seedBrokers (if the broker was a seed broker) - removes it from the brokers map (otherwise) In the producer and consumer where we used to call disconnectBroker: - We now call `Close` directly on the broker. - The broker we are dealing with is not a seed broker, so the seedBrokers do not need rotating, and I don't think it's a problem that it no longer gets added to `deadBrokerAddrs`. - The reason we removed it from the broker map was so that the next request for that broker would trigger a metadata request and reopen the connection. The producer and consumer both manually trigger metadata requests when necessary, and the fact that we now have lazy connection opening means simply closing it (which we do, see first bullet) is enough to cause the connection to reopen the next time it is requested, even if no metadata refresh is requested.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of opening a connection to all brokers immediately upon receiving their
information in metadata, wait until we are asked for them either via a call to
Leaderor a call toany.It seems simple enough (simpler than I was expecting actually) but I'm a bit wary, since it changes the internal semantics so much. Strong opinions and careful review please.
@Shopify/kafka