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

Do network connections and writes in KafkaClient.poll() #1729

Merged
merged 6 commits into from
Mar 8, 2019
Next Next commit
Add BrokerConnection.send_pending_requests to support async network s…
…ends
  • Loading branch information
dpkp authored and jeffwidman committed Mar 8, 2019
commit 3c873794f04d968895b0452a26ac061862686abf
49 changes: 30 additions & 19 deletions kafka/conn.py
Original file line number Diff line number Diff line change
Expand Up @@ -733,47 +733,58 @@ def close(self, error=None):
future.failure(error)
self.config['state_change_callback'](self)

def send(self, request):
"""send request, return Future()

Can block on network if request is larger than send_buffer_bytes
"""
def send(self, request, blocking=True):
"""Queue request for async network send, return Future()"""
future = Future()
if self.connecting():
return future.failure(Errors.NodeNotReadyError(str(self)))
elif not self.connected():
return future.failure(Errors.KafkaConnectionError(str(self)))
elif not self.can_send_more():
return future.failure(Errors.TooManyInFlightRequests(str(self)))
return self._send(request)
return self._send(request, blocking=blocking)

def _send(self, request):
def _send(self, request, blocking=True):
assert self.state in (ConnectionStates.AUTHENTICATING, ConnectionStates.CONNECTED)
future = Future()
correlation_id = self._protocol.send_request(request)

# Attempt to replicate behavior from prior to introduction of
# send_pending_requests() / async sends
if blocking:
error = self.send_pending_requests()
if isinstance(error, Exception):
future.failure(error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps add a debug-level log statement here of this error?
It looks like send_pending_requests() already logs most (but not all) errors, so this may be superfluous, but my one thought is that if someone files a ticket, we have a little more visibility/guarantees about the errors they're hitting...

return future

log.debug('%s Request %d: %s', self, correlation_id, request)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this log line be located above the if blocking: line? Since the info seems useful regardless of whether blocking.

Copy link
Owner Author

Choose a reason for hiding this comment

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

In the current implementation we only log this after the request is sent successfully. So I put this after the blocking section to keep it consistent.

if request.expect_response():
sent_time = time.time()
ifr = (correlation_id, future, sent_time)
self.in_flight_requests.append(ifr)
else:
future.success(None)
return future

def send_pending_requests(self):
"""Can block on network if request is larger than send_buffer_bytes"""
if self.state not in (ConnectionStates.AUTHENTICATING,
ConnectionStates.CONNECTED):
return Errors.NodeNotReadyError(str(self))
data = self._protocol.send_bytes()
try:
# In the future we might manage an internal write buffer
# and send bytes asynchronously. For now, just block
# sending each request payload
sent_time = time.time()
total_bytes = self._send_bytes_blocking(data)
if self._sensors:
self._sensors.bytes_sent.record(total_bytes)
return total_bytes
except ConnectionError as e:
log.exception("Error sending %s to %s", request, self)
log.exception("Error sending request data to %s", self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to stop logging the request value?

Copy link
Owner Author

Choose a reason for hiding this comment

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

with this design we only have the encoded bytes at this stage -- we no longer have the original request object. so for that reason i took it out of the log message. This error should be sent down to the future and we can expect that the error handler for the request future will be responsible for logging the details.

error = Errors.KafkaConnectionError("%s: %s" % (self, e))
self.close(error=error)
return future.failure(error)
log.debug('%s Request %d: %s', self, correlation_id, request)

if request.expect_response():
ifr = (correlation_id, future, sent_time)
self.in_flight_requests.append(ifr)
else:
future.success(None)

return future
return error

def can_send_more(self):
"""Return True unless there are max_in_flight_requests_per_connection."""
Expand Down