-
Notifications
You must be signed in to change notification settings - Fork 530
RUBY-3602 Include server address in OperationFailure and BulkWriteError messages #3027
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
Changes from 11 commits
fa03598
5f282dd
241c3b1
0898aea
5af60b7
409e500
c39ff04
07c2b63
9650b51
dd8029b
3c1e89b
6535216
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,10 @@ class ResultCombiner | |
| # @return [ Hash ] results The results hash. | ||
| attr_reader :results | ||
|
|
||
| # @return [ Array<String> ] Deduplicated list of "host:port" addresses of | ||
| # the servers that produced the combined operation results. | ||
| attr_reader :server_addresses | ||
|
|
||
| # Create the new result combiner. | ||
| # | ||
| # @api private | ||
|
|
@@ -39,6 +43,7 @@ class ResultCombiner | |
| def initialize | ||
| @results = {} | ||
| @count = 0 | ||
| @server_addresses = [] | ||
| end | ||
|
|
||
| # Adds a result to the overall results. | ||
|
|
@@ -68,6 +73,8 @@ def combine!(result, count) | |
| combine_errors!(result) | ||
| @count += count | ||
| @acknowledged = result.acknowledged? | ||
| seed = result.connection_description&.address&.seed | ||
| @server_addresses << seed if seed && !@server_addresses.include?(seed) | ||
|
Comment on lines
+76
to
+77
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving this as-is. The complexity here is O(n²) in the number of distinct server addresses, not the number of batches — dedup keeps n tiny. A bulk write targets a primary in a replset or a handful of shards (typically n ≤ 10), so even thousands of combined batches do only a few thousand comparisons total. Switching to a |
||
| end | ||
|
|
||
| # Get the final result. | ||
|
|
@@ -78,7 +85,7 @@ def combine!(result, count) | |
| # | ||
| # @since 2.1.0 | ||
| def result | ||
| BulkWrite::Result.new(results, @acknowledged).validate! | ||
| BulkWrite::Result.new(results, @acknowledged, @server_addresses).validate! | ||
| end | ||
|
|
||
| private | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,17 +34,26 @@ class BulkWriteError < Error | |
| # @return [ BSON::Document ] result The error result. | ||
| attr_reader :result | ||
|
|
||
| # @return [ Array<String> ] Deduplicated list of "host:port" addresses of | ||
| # the servers that produced this bulk write error. Empty when no | ||
| # addresses were supplied. | ||
| attr_reader :server_addresses | ||
|
|
||
| # Instantiate the new exception. | ||
| # | ||
| # @example Instantiate the exception. | ||
| # Mongo::Error::BulkWriteError.new(response) | ||
| # | ||
| # @param [ Hash ] result A processed response from the server | ||
| # reporting results of the operation. | ||
| # @param [ Array<String | Mongo::Address | Mongo::Server::Description> ] | ||
| # server_addresses Addresses of the servers that produced this error. | ||
| # Entries are normalized to "host:port" strings. | ||
| # | ||
| # @since 2.0.0 | ||
| def initialize(result) | ||
| def initialize(result, server_addresses: nil) | ||
| @result = result | ||
| @server_addresses = normalize_server_addresses(server_addresses) | ||
|
|
||
|
Comment on lines
+54
to
57
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Fixed in 6535216 — |
||
| # Exception constructor behaves differently for a nil argument and | ||
| # for no argument. Avoid passing nil explicitly. | ||
|
|
@@ -93,8 +102,27 @@ def build_message | |
|
|
||
| fragment = "Multiple errors: #{fragment}" if errors.length > 1 | ||
|
|
||
| if Mongo.include_server_address_in_errors && @server_addresses.any? | ||
| fragment = "#{fragment} (on #{@server_addresses.uniq.join(', ')})" | ||
| end | ||
|
|
||
| fragment | ||
| end | ||
|
|
||
| def normalize_server_addresses(value) | ||
| return [] if value.nil? | ||
|
|
||
| Array(value).filter_map do |entry| | ||
| case entry | ||
| when String then entry | ||
| when Mongo::Address then entry.seed | ||
| when Mongo::Server::Description then entry.address&.seed | ||
| else | ||
| raise ArgumentError, | ||
| "server_addresses entries must be String, Mongo::Address, or Mongo::Server::Description; got #{entry.class}" | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'spec_helper' | ||
|
|
||
| describe Mongo::BulkWrite::ResultCombiner do | ||
| describe 'server_addresses accumulation' do | ||
| let(:combiner) { described_class.new } | ||
|
|
||
| def stub_op_result(seed) | ||
| description = instance_double( | ||
| Mongo::Server::Description, | ||
| address: Mongo::Address.new(seed) | ||
| ) | ||
| result = double('op_result') | ||
| allow(result).to receive(:write_concern_error?).and_return(false) | ||
| allow(result).to receive(:acknowledged?).and_return(true) | ||
| allow(result).to receive(:aggregate_write_errors).and_return(nil) | ||
| allow(result).to receive(:aggregate_write_concern_errors).and_return(nil) | ||
| allow(result).to receive(:validate!).and_return(true) | ||
| allow(result).to receive(:respond_to?).and_return(false) | ||
| allow(result).to receive(:connection_description).and_return(description) | ||
| result | ||
| end | ||
|
|
||
| it 'collects unique seeds from combined results' do | ||
| combiner.combine!(stub_op_result('h1:27017'), 1) | ||
| combiner.combine!(stub_op_result('h2:27017'), 1) | ||
| combiner.combine!(stub_op_result('h1:27017'), 1) | ||
|
|
||
| final = combiner.result | ||
| expect(final.server_addresses).to contain_exactly('h1:27017', 'h2:27017') | ||
| end | ||
|
|
||
| it 'defaults to empty when no results combined' do | ||
| expect(combiner.server_addresses).to eq([]) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6535216. The initializer now stores
Array(server_addresses).compact.uniq, so the public reader honors the "deduplicated" docstring regardless of what callers pass in.