Conversation
…tors control over the *sql.Context creation.
| } | ||
| //handler = NewHandler_(e, sm, cfg.ConnReadTimeout, cfg.DisableClientMultiStatements, cfg.MaxLoggedQueryLen, cfg.EncodeLoggedQuery, listener) | ||
| return newServerFromHandler(cfg, e, sm, handler, listener) | ||
| func NewServer(cfg Config, e *sqle.Engine, ctxFactory sql.ContextFactory, sb SessionBuilder, listener ServerEventListener) (*Server, error) { |
There was a problem hiding this comment.
I don't love having both a ctxFactory and a SessionBuilder here, the reality is these two responsibilities are closely related if not overlapping. Ideally this would be a single real interface with two methods that a single type could implement. This isn't a deal breaker but as long as you're monkeying around in here it might be worthwhile to see what it would take to get there.
There was a problem hiding this comment.
Very much agree. This ContextFactory is a pretty weird integration point as of now. There are other ways of achieving what we need. For example, we could allow an integrator to pass along a []sql.ContextOption instead, and make a new context option like WithContextTransform(func(context.Context) context.Context). The reality is that the lifecycle around all these things is really subtle right now, and making changes is somewhat fragile and far reaching.
This change as is is useful to me and I have doltgres and dolt fixes ready for it, so I'm going to go ahead and land it, but I may revisit it soon.
2 participants
No description provided.