GODRIVER-2810 Switch to polling monitoring when running within a FaaS environment #1376
Conversation
prestonvasquez
had a problem deploying
to
api-report
GitHub Actions
Failure
prestonvasquez
had a problem deploying
to
api-report
GitHub Actions
Failure
prestonvasquez
had a problem deploying
to
api-report
GitHub Actions
Failure
prestonvasquez
temporarily deployed
to
api-report
GitHub Actions
Inactive
API Change Report./mongo/optionscompatible changes(*ClientOptions).SetServerMonitoringMode: added ./x/mongo/driver/connstringcompatible changesConnString.ServerMonitoringMode: added |
prestonvasquez
temporarily deployed
to
api-report
GitHub Actions
Inactive
prestonvasquez
had a problem deploying
to
api-report
GitHub Actions
Failure
| @@ -125,99 +124,6 @@ func (h *Hello) Result(addr address.Address) description.Server { | |||
| return description.NewServer(addr, bson.Raw(h.res)) | |||
| } | |||
|
|
|||
| const ( | |||
There was a problem hiding this comment.
Moved all of this logic to the driverutil internal package so it can be shared across multiple x packages without duplication.
prestonvasquez
had a problem deploying
to
api-report
GitHub Actions
Failure
prestonvasquez
temporarily deployed
to
api-report
GitHub Actions
Inactive
prestonvasquez
dismissed stale reviews from qingyang-hu and matthewdale
via
d2798ef
| return events[0], events[:1], nil | ||
| } | ||
|
|
||
| func verifySDAMEvents(client *clientEntity, expectedEvents *expectedEvents) error { |
There was a problem hiding this comment.
@qingyang-hu @matthewdale The addition of the 1.17 schema in this ticket and the UTR updates missed in the implementation of mongodb/specifications#1291 require this verification function.
| @@ -206,6 +226,7 @@ type ClientOptions struct { | |||
| RetryReads *bool | |||
| RetryWrites *bool | |||
| ServerAPIOptions *ServerAPIOptions | |||
| ServerMonitoringMode *string | |||
There was a problem hiding this comment.
We define the expected values of ServerMonitoringMode, so we should define a string value for "not set" instead of using a pointer. I recommend defining empty string as "not set".
There was a problem hiding this comment.
We also define expected values for the read/write concerns. Not using a pointer here deviates from the existing pattern, which seems to intend that data in an options struct be nullable. We don't expect users to construct this data at the struct level:
Each option can be set through setter functions.
Would you mind expanding on this? And what is meant by "we should define a string value for 'not set'"? If we do go the non-pointer route, I don't think we should extend the "ServerMonitoringMode*" constants to include a "ServerMonitoringModeNotSet" case, it's not defined in the specifications and I'm not sure how it would help a user.
There was a problem hiding this comment.
I was mostly reacting to the additional logic for handling pointers, which is unnecessary if the zero value can represent "not set". However, you're correct that the strong existing pattern in ClientOptions is to use pointers and nil for "not set". This PR doesn't seem like the right place to change that pattern. Consider my original comment resolved.
| @@ -206,6 +226,7 @@ type ClientOptions struct { | |||
| RetryReads *bool | |||
| RetryWrites *bool | |||
| ServerAPIOptions *ServerAPIOptions | |||
| ServerMonitoringMode *string | |||
There was a problem hiding this comment.
I was mostly reacting to the additional logic for handling pointers, which is unnecessary if the zero value can represent "not set". However, you're correct that the strong existing pattern in ClientOptions is to use pointers and nil for "not set". This PR doesn't seem like the right place to change that pattern. Consider my original comment resolved.
prestonvasquez
dismissed stale reviews from qingyang-hu and matthewdale
via
b686f7c
| @@ -28,6 +28,8 @@ RUN export DEBIAN_FRONTEND=noninteractive && \ | |||
| tzdata \ | |||
| gpg \ | |||
| apt-utils \ | |||
| libc6-dev \ | |||
| gcc \ | |||
There was a problem hiding this comment.
C compiler is required to run a race test on non-darwin, see requirements: https://go.dev/doc/articles/race_detector#Requirements
| // connMu guards connecting and disconnecting. This is necessary since | ||
| // disconnecting will await the cancelation of a started connection. The | ||
| // use case for rttMonitor.connect needs to be goroutine safe. | ||
| connMu sync.Mutex |
There was a problem hiding this comment.
This PR requires that the rttMonitor's connection be established in a separate thread than the one server.Disconnect() is called in:
Because of this, it's possible that a disconnect could occur before a connection actually takes place, P_0. This would make any call to the shared wait group unsafe. connMu guards this case.
There should be no locking contention with this solution, the P_0 case is extremely difficult to force and there should only be one call to connect per server connection.
There was a problem hiding this comment.
@qingyang-hu @matthewdale I just caught a race condition in the logic. Hopefully this is the last review required
… environment (mongodb#1376) (cherry picked from commit 99bdb94)
GODRIVER-2810
Summary
Revert SDAM monitoring process to use the legacy polling mechanism when a FaaS runtime environment is detected.
Background & Motivation
The streaming protocol used by SDAM monitoring when communicating with MongoDB 4.4+ clusters can result in socket timeouts when the underlying runtime environment resumes after being frozen/paused then (ex: Lambda).