isapego commented on a change in pull request #45:
URL:
https://github.com/apache/ignite-python-thin-client/pull/45#discussion_r669399149
##########
File path: pyignite/connection/connection.py
##########
@@ -28,6 +29,8 @@
CLIENT_STATUS_AUTH_FAILURE = 2000
+logger = logging.getLogger('.'.join(__name__.split('.')[:-1]))
Review comment:
Is it a good idea to make it global?
##########
File path: pyignite/connection/connection.py
##########
@@ -78,21 +81,53 @@ def protocol_context(self):
return self.client.protocol_context
def _process_handshake_error(self, response):
- error_text = f'Handshake error: {response.message}'
# if handshake fails for any reason other than protocol mismatch
# (i.e. authentication error), server version is 0.0.0
+ if response.client_status == CLIENT_STATUS_AUTH_FAILURE:
+ raise AuthenticationError(response.message)
+
protocol_version = self.client.protocol_context.version
server_version = (response.version_major, response.version_minor,
response.version_patch)
-
+ error_text = f'Handshake error: {response.message}'
if any(server_version):
error_text += f' Server expects binary protocol version ' \
f'{server_version[0]}.{server_version[1]}.{server_version[2]}. ' \
f'Client provides ' \
f'{protocol_version[0]}.{protocol_version[1]}.{protocol_version[2]}.'
- elif response.client_status == CLIENT_STATUS_AUTH_FAILURE:
- raise AuthenticationError(error_text)
raise HandshakeError(server_version, error_text)
+ def _on_handshake_start(self):
+ if logger.isEnabledFor(logging.DEBUG):
+ logger.debug("Connecting to node(address=%s, port=%d) with
protocol context %s",
+ self.host, self.port, self.client.protocol_context)
+
+ def _on_handshake_success(self, result):
+ features = BitmaskFeature.from_array(result.get('features', None))
+ self.client.protocol_context.features = features
+ self.uuid = result.get('node_uuid', None) # version-specific (1.4+)
+ self.failed = False
+
+ if logger.isEnabledFor(logging.DEBUG):
+ logger.debug("Connected to node(address=%s, port=%d, node_uuid=%s)
with protocol context %s",
+ self.host, self.port, self.uuid,
self.client.protocol_context)
+
+ def _on_handshake_fail(self, err):
+ if isinstance(err, AuthenticationError):
+ logger.error("Authentication failed while connecting to
node(address=%s, port=%d): %s",
+ self.host, self.port, err)
+ if isinstance(err, connection_errors):
+ logger.error("Failed to perform handshake, connection to
node(address=%s, port=%d) "
+ "with protocol context %s failed: %s",
+ self.host, self.port, self.client.protocol_context,
err, exc_info=True)
Review comment:
What about other kind of errors? I believe, we should log them too.
##########
File path: pyignite/connection/connection.py
##########
@@ -78,21 +81,53 @@ def protocol_context(self):
return self.client.protocol_context
def _process_handshake_error(self, response):
- error_text = f'Handshake error: {response.message}'
# if handshake fails for any reason other than protocol mismatch
# (i.e. authentication error), server version is 0.0.0
+ if response.client_status == CLIENT_STATUS_AUTH_FAILURE:
+ raise AuthenticationError(response.message)
+
protocol_version = self.client.protocol_context.version
server_version = (response.version_major, response.version_minor,
response.version_patch)
-
+ error_text = f'Handshake error: {response.message}'
if any(server_version):
error_text += f' Server expects binary protocol version ' \
f'{server_version[0]}.{server_version[1]}.{server_version[2]}. ' \
f'Client provides ' \
f'{protocol_version[0]}.{protocol_version[1]}.{protocol_version[2]}.'
- elif response.client_status == CLIENT_STATUS_AUTH_FAILURE:
- raise AuthenticationError(error_text)
raise HandshakeError(server_version, error_text)
+ def _on_handshake_start(self):
+ if logger.isEnabledFor(logging.DEBUG):
+ logger.debug("Connecting to node(address=%s, port=%d) with
protocol context %s",
+ self.host, self.port, self.client.protocol_context)
+
+ def _on_handshake_success(self, result):
+ features = BitmaskFeature.from_array(result.get('features', None))
+ self.client.protocol_context.features = features
+ self.uuid = result.get('node_uuid', None) # version-specific (1.4+)
+ self.failed = False
+
+ if logger.isEnabledFor(logging.DEBUG):
+ logger.debug("Connected to node(address=%s, port=%d, node_uuid=%s)
with protocol context %s",
+ self.host, self.port, self.uuid,
self.client.protocol_context)
+
+ def _on_handshake_fail(self, err):
+ if isinstance(err, AuthenticationError):
+ logger.error("Authentication failed while connecting to
node(address=%s, port=%d): %s",
+ self.host, self.port, err)
+ if isinstance(err, connection_errors):
+ logger.error("Failed to perform handshake, connection to
node(address=%s, port=%d) "
+ "with protocol context %s failed: %s",
+ self.host, self.port, self.client.protocol_context,
err, exc_info=True)
+
+ def _on_connection_lost(self, err=None, expected=False):
+ if expected and logger.isEnabledFor(logging.DEBUG):
+ logger.debug("Connection closed to node(address=%s, port=%d,
node_uuid=%s)",
+ self.host, self.port, self.uuid)
+ else:
+ logger.error("Connection lost to node(address=%s, port=%d,
node_uuid=%s): %s",
Review comment:
Lost connection to the node is not an error. It should be `warning` or
`info` I believe. Client can continue working on lost connection and user
should expect that such kind of event may occur.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
