moapis / multidb Goto Github PK

Some files do not have a proper copyright header notice. All source files to be checked and updated accordingly.

In #11, the connErr field was removed. Also, re-connection logic is bound to be removed (#14). By implementing the possibility of adding an error callback function during Node creation, package consumers would be able to monitor and log Node specific errors. As those errors might never get reported during MultiQuery calls, if at least one query on a Node succeeds.

Node's and MultiNode's BeginTx() do not implement boil.Beginner as it requires a *sql.Tx return type.

A design change has been requested upstream: volatiletech/sqlboiler#641

In the process of #16: Simplify or remove drivers, the old examples became incorrect and had to be removed.

New examples reflecting the new package interface should be written.

After removing the reconnection logic in #14, the statistics counter can also be removed.

Don't nil the DB pointer when connection is closed. Instead, let de DB driver handle the closed situation and let it return proper errors.

At the moment, if a context is canceled by the caller, we are counting it as a connection error for the node. After enough of such cancels, the node is disregarded.

Package context defines the Canceled variable, against which we can test.

Testing is currently done here:

This depends on #4, and might be implemented simultanious.

Some constants are defined like these:

The more idiomatic way is to declare global variables, using errors.New(). At the moment the errors are created inside the function scope. This means equality can only be tested based on strings. Instead, this package should offer errors which are easily investigated using errors.Is().

Reconnection is handled by the SQL driver. Also, if an error is returned on first Connect, it mostly mean an error was made in de dsn, not the actual connection. (SQL library only connects when a Connection needs to be Acquired the first time).

Since Go 1.13, errors can be wrapped. Helper methods are provided to "Unwrap" and test for the originating error. I would like to refactor this package to use the Is method from the errors package, in order to provide more robust testing at:

Previously, connections were fully managed by MultiDB. This way it also had full controll over the status of the Master node. However, we are dropping connection management in favor of the standard sql library own (re) connection logic. We have dropped Node specific error statistics in favor of better performance.

This means that, in case the Master starts failing (and another Node has been promoted) this package won't be aware of this. Simply put, if the caller obtains a Master node through MultiDB.Master() or MultiDB.MasterTX(), it should decide itself if a returned error is serious enough to find a new Master.

I would like to add a utility function that can be called in case of errors, that would help with this decision making and optionally even triggering SelectMaster() if needed.

After we drop re-connection logic (#14), it might be feasible to greatly simplify or remove the drivers.Configurator interface. Perhaps make it possible to set DB objects directly. This way we can support all driver implementations without (too much) hassle.

In all QueryRow variants, errors are deferred until row.Scan. Therefore, this package cannot monitor such errors.

A related feature request is opened at: golang/go#35804

Overwriting the DB potentially leaves it dangling with open connections in its pool and it will not get garbage collected. In the long run, this can lead to memory leaks, if the caller does not keep track of re-configured databases and proper closing of old entities.

Opening and Closing of DB is not in the scope of this packages. However, we can aide the process by returning the discarded DB from mdb.Add(), so that downstream can choose to close it.

Currently the last encountered error on a DB Node is stored in the connErr field. The last occurred error can be received by the ConnErr function. Recently, Go vet started to complain about reflect.DeepEqual on the error in unit test.

Practically speaking:

• Setting and getting of the error added complexity to the code and it would be cleaner without
• If a query cannot complete due to multiple or similar errors, an error is properly returned
• I don't see a practical point where package consumers will investigate individual Nodes after queries. This would only add in code complexity on the caller's side
• Queries on a Node are executed concurrently. A mutex is preventing race conditions on the connErr, however it does not guarantee which error is returned from the ConnErr method. A caller might be expecting the error status of the latest called Query, but it may was well return the latest terminated query. There is no way query calls are matched against the connErr state. Meaning this can lead to misunderstanding of the information and bugs.

When the opts argument is nil, a default &sql.TxOptions{ReadOnly: true} should be set.

Currently the sqlboiler module is pulling lots of dependencies in go.sum. This module is only really used in interface compliance verification like:

It would be a "cleaner" impression of the project if we could move this in a sub-module for testing only. Then, we let CI bump the sub-module to the latest commit hash on every push to verify compliance.