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

Adopts blocklist/allowlist as suitable alternatives for the harmful terms blacklist/allowlist #312

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class ErrorHandler < ::Makara::ErrorHandler
HARSH_ERRORS = [
'ActiveRecord::RecordNotUnique',
'ActiveRecord::InvalidForeignKey',
'Makara::Errors::BlacklistConnection'
'Makara::Errors::BlocklistConnection'
].map(&:freeze).freeze

CONNECTION_MATCHERS = [
Expand Down
24 changes: 21 additions & 3 deletions lib/makara.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,29 @@ module Makara

module Errors
autoload :MakaraError, 'makara/errors/makara_error'
autoload :AllConnectionsBlacklisted, 'makara/errors/all_connections_blacklisted'
autoload :BlacklistConnection, 'makara/errors/blacklist_connection'
autoload :AllConnectionsBlocklisted, 'makara/errors/all_connections_blocklisted'
autoload :BlocklistConnection, 'makara/errors/blocklist_connection'
autoload :NoConnectionsAvailable, 'makara/errors/no_connections_available'
autoload :BlacklistedWhileInTransaction, 'makara/errors/blacklisted_while_in_transaction'
autoload :BlocklistedWhileInTransaction, 'makara/errors/blocklisted_while_in_transaction'
autoload :InvalidShard, 'makara/errors/invalid_shard'

DEPRECATED_CLASSES = {
:AllConnectionsBlacklisted => AllConnectionsBlocklisted,
:BlacklistConnection => BlocklistConnection,
:BlacklistedWhileInTransaction => BlocklistedWhileInTransaction,
}

def self.const_missing(const_name)
if DEPRECATED_CLASSES.key? const_name
replacement = DEPRECATED_CLASSES.fetch(const_name)

warn "Makara::Errors::#{const_name} is deprecated. Switch to #{replacement}"

replacement
else
super
end
end
end

module Logging
Expand Down
8 changes: 5 additions & 3 deletions lib/makara/config_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
# another: 'top level variable'
# makara:
# primary_ttl: 3
# blacklist_duration: 20
# blacklist_duration: 20 # Deprecated in favor of 'blocklist_duration'
# blocklist_duration: 20
# connections:
# - role: 'master' # Deprecated in favor of 'primary'
# - role: 'primary'
Expand All @@ -23,7 +24,7 @@ module Makara
class ConfigParser
DEFAULTS = {
primary_ttl: 5,
blacklist_duration: 30,
blocklist_duration: 30,
sticky: true
}

Expand All @@ -34,7 +35,8 @@ class ConfigParser
master_strategy: :primary_strategy,
master_shard_aware: :primary_shard_aware,
master_default_shard: :primary_default_shard,
master_ttl: :primary_ttl
master_ttl: :primary_ttl,
blacklist_duration: :blocklist_duration,
}.freeze

ConnectionUrlResolver =
Expand Down
30 changes: 15 additions & 15 deletions lib/makara/connection_wrapper.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'active_support/core_ext/hash/keys'

# Makara::ConnectionWrapper wraps the instance of an underlying connection.
# The wrapper provides methods for tracking blacklisting and individual makara configurations.
# The wrapper provides methods for tracking blocklisting and individual makara configurations.
# Upon creation, the wrapper defines methods in the underlying object giving it access to the
# Makara::Proxy.

Expand All @@ -18,7 +18,7 @@ def initialize(proxy, connection, config)
@proxy = proxy

if connection.nil?
_makara_blacklist!
_makara_blocklist!
else
_makara_decorate_connection(connection)
end
Expand All @@ -38,25 +38,25 @@ def _makara_shard_id
@config[:shard_id]
end

# has this node been blacklisted?
def _makara_blacklisted?
Copy link
Author

Choose a reason for hiding this comment

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

Is it necessary to provide some kind of backward compatible support for these old methods? I would have assumed "yes" because they are public methods, but the _ prefixes in the method names indicate to me that it is not intended for users to call or override these methods themselves.

@blacklisted_until.present? && @blacklisted_until.to_i > Time.now.to_i
# has this node been blocklisted?
def _makara_blocklisted?
@blocklisted_until.present? && @blocklisted_until.to_i > Time.now.to_i
end

def _makara_in_transaction?
@connection && @connection.open_transactions > 0
end

# blacklist this node for @config[:blacklist_duration] seconds
def _makara_blacklist!
# blocklist this node for @config[:blocklist_duration] seconds
def _makara_blocklist!
@connection.disconnect! if @connection
@connection = nil
@blacklisted_until = Time.now.to_i + @config[:blacklist_duration] unless @config[:disable_blacklist]
@blocklisted_until = Time.now.to_i + @config[:blocklist_duration] unless @config[:disable_blocklist]
end

# release the blacklist
def _makara_whitelist!
@blacklisted_until = nil
# release the blocklist
def _makara_allowlist!
@blocklisted_until = nil
end

# custom error messages
Expand All @@ -66,7 +66,7 @@ def _makara_custom_error_matchers

def _makara_connected?
_makara_connection.present?
rescue Makara::Errors::BlacklistConnection
rescue Makara::Errors::BlocklistConnection
false
end

Expand All @@ -75,13 +75,13 @@ def _makara_connection

if current
current
else # blacklisted connection or initial error
else # blocklisted connection or initial error
new_connection = @proxy.graceful_connection_for(@config)

# Already wrapped because of initial failure
if new_connection.is_a?(Makara::ConnectionWrapper)
_makara_blacklist!
raise Makara::Errors::BlacklistConnection.new(self, new_connection.initial_error)
_makara_blocklist!
raise Makara::Errors::BlocklistConnection.new(self, new_connection.initial_error)
else
@connection = new_connection
_makara_decorate_connection(new_connection)
Expand Down
2 changes: 1 addition & 1 deletion lib/makara/error_handler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def handle(connection)
protected

def gracefully(connection, e)
err = Makara::Errors::BlacklistConnection.new(connection, e)
err = Makara::Errors::BlocklistConnection.new(connection, e)
::Makara::Logging::Logger.log("Gracefully handling: #{err}")
raise err
end
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
module Makara
module Errors
class AllConnectionsBlacklisted < MakaraError
class AllConnectionsBlocklisted < MakaraError
def initialize(pool, errors)
errors = [*errors]
messages = errors.empty? ? 'No error details' : errors.map(&:message).join(' -> ')
super "[Makara/#{pool.role}] All connections are blacklisted -> " + messages
super "[Makara/#{pool.role}] All connections are blocklisted -> " + messages
end
end
end
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module Makara
module Errors
class BlacklistConnection < MakaraError
class BlocklistConnection < MakaraError
attr_reader :original_error

def initialize(connection, error)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
module Makara
module Errors
class BlacklistedWhileInTransaction < MakaraError
class BlocklistedWhileInTransaction < MakaraError
attr_reader :role

def initialize(role)
@role = role
super "[Makara] Blacklisted while in transaction in the #{role} pool"
super "[Makara] Blocklisted while in transaction in the #{role} pool"
end
end
end
Expand Down
44 changes: 22 additions & 22 deletions lib/makara/pool.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class Pool
# there are cases when we understand the pool is busted and we essentially want to skip
# all execution
attr_accessor :disabled
attr_reader :blacklist_errors
attr_reader :blocklist_errors
attr_reader :role
attr_reader :connections
attr_reader :strategy
Expand All @@ -20,7 +20,7 @@ def initialize(role, proxy)
@role = role == "master" ? "primary" : role
@proxy = proxy
@connections = []
@blacklist_errors = []
@blocklist_errors = []
@disabled = false
if proxy.shard_aware_for(role)
@strategy = Makara::Strategies::ShardAware.new(self)
Expand All @@ -31,9 +31,9 @@ def initialize(role, proxy)
end
end

def completely_blacklisted?
def completely_blocklisted?
@connections.each do |connection|
return false unless connection._makara_blacklisted?
return false unless connection._makara_blocklisted?
end
true
end
Expand Down Expand Up @@ -66,7 +66,7 @@ def send_to_all(method, *args, &block)
errors = []

@connections.each do |con|
next if con._makara_blacklisted?
next if con._makara_blocklisted?

begin
ret = @proxy.error_handler.handle(con) do
Expand All @@ -78,15 +78,15 @@ def send_to_all(method, *args, &block)
end

one_worked = true
rescue Makara::Errors::BlacklistConnection => e
rescue Makara::Errors::BlocklistConnection => e
errors.insert(0, e)
con._makara_blacklist!
con._makara_blocklist!
end
end

if !one_worked
if connection_made?
raise Makara::Errors::AllConnectionsBlacklisted.new(self, errors)
raise Makara::Errors::AllConnectionsBlocklisted.new(self, errors)
else
raise Makara::Errors::NoConnectionsAvailable.new(@role) unless @disabled
end
Expand All @@ -95,46 +95,46 @@ def send_to_all(method, *args, &block)
ret
end

# Provide a connection that is not blacklisted and connected. Handle any errors
# Provide a connection that is not blocklisted and connected. Handle any errors
# that may occur within the block.
def provide
attempt = 0
begin
provided_connection = self.next

# nil implies that it's blacklisted
# nil implies that it's blocklisted
if provided_connection

value = @proxy.error_handler.handle(provided_connection) do
yield provided_connection
end

@blacklist_errors = []
@blocklist_errors = []

value

# if we've made any connections within this pool, we should report the blackout.
elsif connection_made?
err = Makara::Errors::AllConnectionsBlacklisted.new(self, @blacklist_errors)
@blacklist_errors = []
err = Makara::Errors::AllConnectionsBlocklisted.new(self, @blocklist_errors)
@blocklist_errors = []
raise err
else
raise Makara::Errors::NoConnectionsAvailable.new(@role) unless @disabled
end

# when a connection causes a blacklist error within the provided block, we blacklist it then retry
rescue Makara::Errors::BlacklistConnection => e
@blacklist_errors.insert(0, e)
# when a connection causes a blocklist error within the provided block, we blocklist it then retry
rescue Makara::Errors::BlocklistConnection => e
@blocklist_errors.insert(0, e)
in_transaction = self.role == "primary" && provided_connection._makara_in_transaction?
provided_connection._makara_blacklist!
raise Makara::Errors::BlacklistedWhileInTransaction.new(@role) if in_transaction
provided_connection._makara_blocklist!
raise Makara::Errors::BlocklistedWhileInTransaction.new(@role) if in_transaction

attempt += 1
if attempt < @connections.length
retry
elsif connection_made?
err = Makara::Errors::AllConnectionsBlacklisted.new(self, @blacklist_errors)
@blacklist_errors = []
err = Makara::Errors::AllConnectionsBlocklisted.new(self, @blocklist_errors)
@blocklist_errors = []
raise err
else
raise Makara::Errors::NoConnectionsAvailable.new(@role) unless @disabled
Expand All @@ -149,9 +149,9 @@ def connection_made?
@connections.any?(&:_makara_connected?)
end

# Get the next non-blacklisted connection. If the proxy is setup
# Get the next non-blocklisted connection. If the proxy is setup
# to be sticky, provide back the current connection assuming it is
# not blacklisted.
# not blocklisted.
def next
if @proxy.sticky && (curr = @strategy.current)
curr
Expand Down
16 changes: 8 additions & 8 deletions lib/makara/proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,14 @@ def graceful_connection_for(config)
@error_handler.handle(fake_wrapper) do
connection_for(config)
end
rescue Makara::Errors::BlacklistConnection => e
rescue Makara::Errors::BlocklistConnection => e
fake_wrapper.initial_error = e.original_error
fake_wrapper
end

def disconnect!
send_to_all(:disconnect!)
rescue ::Makara::Errors::AllConnectionsBlacklisted, ::Makara::Errors::NoConnectionsAvailable
rescue ::Makara::Errors::AllConnectionsBlocklisted, ::Makara::Errors::NoConnectionsAvailable
# all connections are already down, nothing to do here
end

Expand All @@ -186,7 +186,7 @@ def any_connection
yield con
end
end
rescue ::Makara::Errors::AllConnectionsBlacklisted, ::Makara::Errors::NoConnectionsAvailable
rescue ::Makara::Errors::AllConnectionsBlocklisted, ::Makara::Errors::NoConnectionsAvailable
begin
@primary_pool.disabled = true
@replica_pool.provide do |con|
Expand Down Expand Up @@ -215,13 +215,13 @@ def appropriate_pool(method_name, args)
# for testing purposes
pool = _appropriate_pool(method_name, args)
yield pool
rescue ::Makara::Errors::AllConnectionsBlacklisted, ::Makara::Errors::NoConnectionsAvailable => e
rescue ::Makara::Errors::AllConnectionsBlocklisted, ::Makara::Errors::NoConnectionsAvailable => e
if pool == @primary_pool
@primary_pool.connections.each(&:_makara_whitelist!)
@replica_pool.connections.each(&:_makara_whitelist!)
@primary_pool.connections.each(&:_makara_allowlist!)
@replica_pool.connections.each(&:_makara_allowlist!)
Kernel.raise e
else
@primary_pool.blacklist_errors << e
@primary_pool.blocklist_errors << e
retry
end
end
Expand All @@ -240,7 +240,7 @@ def _appropriate_pool(method_name, args)
@primary_pool

# all replicas are down (or empty)
elsif @replica_pool.completely_blacklisted?
elsif @replica_pool.completely_blocklisted?
stick_to_primary(method_name, args)
@primary_pool

Expand Down
4 changes: 2 additions & 2 deletions lib/makara/strategies/priority_failover.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ def next
nil
end

# return the connection if it's not blacklisted
# return the connection if it's not blocklisted
# otherwise return nil
# optionally, store the position and context we're returning
def safe_value(idx)
con = @weighted_connections[idx]
return nil unless con
return nil if con._makara_blacklisted?
return nil if con._makara_blocklisted?

con
end
Expand Down
Loading