Add support for multiple drivers in same session #7
Conversation
There was a problem hiding this comment.
This was mainly code movement into the session struct, right?
Reviewable status: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @andy-kimball)
session.go, line 60 at r1 (raw file):
// or write a new recording of the given name in a file with the given path // name. func NewSession(pathName, recordingName string) *session {
Does this need to be exported? I'm pretty sure no given that session itself isn't exported. One of the linters in CRDB actually complains about returning an unexported type from an exported function due to the inability to actually use the type for anything useful.
Allow connections to be made using different SQL drivers in the same copyist session. Records generated by different drivers are stored the same way as if there was just a single driver - in the same recording file. The main thing preventing this from working before was that the proxyDriver was the top-level object which stored session state. Now all session state is broken out into a new session object which can manage multiple drivers. Note also that this change means that Register can no longer take a resetDB argument, since there should not be a different reset function for each driver. A new SetSessionInit function can now be used to set the callback function that is invoked each time a new copyist session is started. This last change does break API backwards compatibility. But since I don't think copyist is widely used, I'm not going to add "/v2" to the import path, even though that's not allowed according to the SemVer spec.
There was a problem hiding this comment.
Yes, refactoring so that we can have 1:MANY session:drivers instead of 1:1.
Reviewable status: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @petermattis)
session.go, line 60 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Does this need to be exported? I'm pretty sure no given that
sessionitself isn't exported. One of the linters in CRDB actually complains about returning an unexported type from an exported function due to the inability to actually use the type for anything useful.
Was mistake on my part. Fixed.
Allow connections to be made using different SQL drivers in the same copyist session. Records generated by different drivers are stored the same way as if there was just a single driver - in the same recording file. The main thing preventing this from working before was that the proxyDriver was the top-level object which stored session state. Now all session state is broken out into a new session object which can manage multiple drivers. Note also that this change means that Register can no longer take a resetDB argument, since there should not be a different reset function for each driver. A new SetSessionInit function can now be used to set the callback function that is invoked each time a new copyist session is started. This last change does break API backwards compatibility. But since I don't think copyist is widely used, I'm not going to add "/v2" to the import path, even though that's not allowed according to the SemVer spec.
Allow connections to be made using different SQL drivers in the same copyist
session. Records generated by different drivers are stored the same way as
if there was just a single driver - in the same recording file.
The main thing preventing this from working before was that the proxyDriver
was the top-level object which stored session state. Now all session state
is broken out into a new session object which can manage multiple drivers.
Note also that this change means that Register can no longer take a resetDB
argument, since there should not be a different reset function for each
driver. A new SetSessionInit function can now be used to set the callback
function that is invoked each time a new copyist session is started.
This last change does break API backwards compatibility. But since I don't
think copyist is widely used, I'm not going to add "/v2" to the import
path, even though that's not allowed according to the SemVer spec.
This change is