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.

Sign up for GitHub

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

Jump to bottom

Remove verbose logging from redis-py/redis/cluster.py #2238

Merged
merged 19 commits into from
Jul 24, 2022
Merged

Remove verbose logging from redis-py/redis/cluster.py #2238

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f259ab5
removed the logging module and its corresponding methods
nialdaly Jun 17, 2022
b19ede7
updated CHANGES
nialdaly Jun 17, 2022
7253f4f
except block for RedisClusterException and BusyLoadingError removed
nialdaly Jun 19, 2022
4e96d4a
Merge branch 'master' into remove-redis-cluster-logs
nialdaly Jun 19, 2022
c5eae34
removed unused import (redis.exceptions.BusyLoadingError)
nialdaly Jun 19, 2022
6deddd9
Merge branch 'remove-redis-cluster-logs' of https://github.com/nialda…
nialdaly Jun 19, 2022
a6721b1
empty commit to re-trigger Actions workflow
nialdaly Jun 19, 2022
fd50baa
replaced BaseException with Exception
nialdaly Jun 21, 2022
f3d8fcb
merged master into remove-redis-cluster-logs
nialdaly Jun 23, 2022
1eabc53
empty commit to re-trigger Actions workflow
nialdaly Jun 23, 2022
284be91
empty commit to re-trigger Actions workflow
nialdaly Jun 23, 2022
db6533e
Merge branch 'master' into remove-redis-cluster-logs
nialdaly Jun 29, 2022
6abd394
Merge branch 'redis:master' into remove-redis-cluster-logs
nialdaly Jul 15, 2022
eaa3435
redundant logic removed
nialdaly Jul 15, 2022
336b4d7
re-trigger pipeline
nialdaly Jul 15, 2022
0407f21
reverted changes
nialdaly Jul 15, 2022
223fa0e
re-trigger pipeline
nialdaly Jul 15, 2022
8680c91
except logic changed
nialdaly Jul 15, 2022
5b56f28
Merge branch 'master' into remove-redis-cluster-logs
nialdaly Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGES
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@

* Remove verbose logging from cluster.py
* Compare commands case-insensitively in the asyncio command parser
* Allow negative `retries` for `Retry` class to retry forever
* Add `items` parameter to `hset` signature
Expand Down
60 changes: 6 additions & 54 deletions redis/cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import copy
import logging
import random
import socket
import sys
Expand All @@ -15,7 +14,6 @@
from redis.exceptions import (
AskError,
AuthenticationError,
BusyLoadingError,
ClusterCrossSlotError,
ClusterDownError,
ClusterError,
Expand All @@ -39,8 +37,6 @@
str_if_bytes,
)

log = logging.getLogger(__name__)


def get_node_name(host: str, port: int) -> str:
return f"{host}:{port}"
Expand Down Expand Up @@ -535,7 +531,6 @@ def __init__(
" RedisCluster(startup_nodes=[ClusterNode('localhost', 6379),"
" ClusterNode('localhost', 6378)])"
)
log.debug(f"startup_nodes : {startup_nodes}")
# Update the connection arguments
# Whenever a new connection is established, RedisCluster's on_connect
# method should be run
Expand Down Expand Up @@ -666,13 +661,8 @@ def set_default_node(self, node):
:return True if the default node was set, else False
"""
if node is None or self.get_node(node_name=node.name) is None:
log.info(
"The requested node does not exist in the cluster, so "
"the default node was not changed."
)
return False
self.nodes_manager.default_node = node
log.info(f"Changed the default cluster node to {node}")
return True

def monitor(self, target_node=None):
Expand Down Expand Up @@ -816,8 +806,6 @@ def _determine_nodes(self, *args, **kwargs):
else:
# get the nodes group for this command if it was predefined
command_flag = self.command_flags.get(command)
if command_flag:
log.debug(f"Target node/s for {command}: {command_flag}")
if command_flag == self.__class__.RANDOM:
# return a random node
return [self.get_random_node()]
Expand All @@ -841,7 +829,6 @@ def _determine_nodes(self, *args, **kwargs):
node = self.nodes_manager.get_node_from_slot(
slot, self.read_from_replicas and command in READ_COMMANDS
)
log.debug(f"Target for {args}: slot {slot}")
return [node]

def _should_reinitialized(self):
Expand Down Expand Up @@ -1019,7 +1006,7 @@ def execute_command(self, *args, **kwargs):
res[node.name] = self._execute_command(node, *args, **kwargs)
# Return the processed result
return self._process_result(args[0], res, **kwargs)
except BaseException as e:
except Exception as e:
if type(e) in self.__class__.ERRORS_ALLOW_RETRY:
# The nodes and slots cache were reinitialized.
# Try again with the new cluster setup.
Expand Down Expand Up @@ -1059,10 +1046,6 @@ def _execute_command(self, target_node, *args, **kwargs):
)
moved = False

log.debug(
f"Executing command {command} on target node: "
f"{target_node.server_type} {target_node.name}"
)
redis_node = self.get_redis_connection(target_node)
connection = get_connection(redis_node, *args, **kwargs)
if asking:
Expand All @@ -1078,11 +1061,9 @@ def _execute_command(self, target_node, *args, **kwargs):
)
return response

except (RedisClusterException, BusyLoadingError, AuthenticationError) as e:
log.exception(type(e))
raise
except AuthenticationError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't catching & re-raising redundant?

Copy link
Contributor Author

@nialdaly nialdaly Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Does this look okay? @utkarshgupta137

raise e
except (ConnectionError, TimeoutError) as e:
log.exception(type(e))
# ConnectionError can also be raised if we couldn't get a
# connection from the pool before timing out, so check that
# this is an actual connection before attempting to disconnect.
Expand All @@ -1101,7 +1082,7 @@ def _execute_command(self, target_node, *args, **kwargs):
# and try again with the new setup
target_node.redis_connection = None
self.nodes_manager.initialize()
raise
raise e
except MovedError as e:
# First, we will try to patch the slots/nodes cache with the
# redirected node output and try again. If MovedError exceeds
Expand All @@ -1111,7 +1092,6 @@ def _execute_command(self, target_node, *args, **kwargs):
# the same client object is shared between multiple threads. To
# reduce the frequency you can set this variable in the
# RedisCluster constructor.
log.exception("MovedError")
self.reinitialize_counter += 1
if self._should_reinitialized():
self.nodes_manager.initialize()
Expand All @@ -1121,29 +1101,21 @@ def _execute_command(self, target_node, *args, **kwargs):
self.nodes_manager.update_moved_exception(e)
moved = True
except TryAgainError:
log.exception("TryAgainError")

if ttl < self.RedisClusterRequestTTL / 2:
time.sleep(0.05)
except AskError as e:
log.exception("AskError")

redirect_addr = get_node_name(host=e.host, port=e.port)
asking = True
except ClusterDownError as e:
log.exception("ClusterDownError")
# ClusterDownError can occur during a failover and to get
# self-healed, we will try to reinitialize the cluster layout
# and retry executing the command
time.sleep(0.25)
self.nodes_manager.initialize()
raise e
except ResponseError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

message = e.__str__()
log.exception(f"ResponseError: {message}")
raise e
nialdaly marked this conversation as resolved.
Show resolved Hide resolved
except BaseException as e:
log.exception("BaseException")
except Exception as e:
if connection:
connection.disconnect()
raise e
Expand Down Expand Up @@ -1280,11 +1252,6 @@ def get_node(self, host=None, port=None, node_name=None):
elif node_name:
return self.nodes_cache.get(node_name)
else:
log.error(
"get_node requires one of the following: "
"1. node name "
"2. host and port"
)
return None

def update_moved_exception(self, exception):
Expand Down Expand Up @@ -1432,7 +1399,6 @@ def initialize(self):
:startup_nodes:
Responsible for discovering other nodes in the cluster
"""
log.debug("Initializing the nodes' topology of the cluster")
self.reset()
tmp_nodes_cache = {}
tmp_slots = {}
Expand Down Expand Up @@ -1460,17 +1426,9 @@ def initialize(self):
)
cluster_slots = str_if_bytes(r.execute_command("CLUSTER SLOTS"))
startup_nodes_reachable = True
except (ConnectionError, TimeoutError) as e:
msg = e.__str__
log.exception(
"An exception occurred while trying to"
" initialize the cluster using the seed node"
f" {startup_node.name}:\n{msg}"
)
except (ConnectionError, TimeoutError):
continue
except ResponseError as e:
log.exception('ReseponseError sending "cluster slots" to redis server')

# Isn't a cluster connection, so it won't parse these
# exceptions automatically
message = e.__str__()
Expand Down Expand Up @@ -2042,12 +2000,6 @@ def _send_cluster_commands(
# If a lot of commands have failed, we'll be setting the
# flag to rebuild the slots table from scratch.
# So MOVED errors should correct themselves fairly quickly.
log.exception(
f"An exception occurred during pipeline execution. "
f"args: {attempt[-1].args}, "
f"error: {type(attempt[-1].result).__name__} "
f"{str(attempt[-1].result)}"
)
self.reinitialize_counter += 1
if self._should_reinitialized():
self.nodes_manager.initialize()
Expand Down