ccl/sqlproxyccl: cleanup PG interceptor APIs #76613
Conversation
|
This change is |
There was a problem hiding this comment.
Note that reviewing individual commits would work best.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @jeffswenson)
pkg/ccl/sqlproxyccl/interceptor/backend_interceptor.go, line 26 at r3 (raw file):
// NOTE: For future improvement, we can use the options pattern here if there's // a need for more than one field. func NewBackendInterceptor(src io.Reader, bufSize int) *BackendInterceptor {
I think keeping this API (i.e. allowing NewBackendInterceptor to take in a bufSize) is reasonable. The caller would just pass in -1 so that the defaults are used.
jaylim-crl
force-pushed
the
jay/cleanup-interceptor-api
branch
from
18ead59 to
db4533b
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/sqlproxyccl/interceptor/backend_interceptor.go, line 54 at r4 (raw file):
// WriteMsg writes the given bytes to the writer dst. This is just a helper // method that invokes Encode to convert the FrontendMessage to bytes. func (bi *BackendInterceptor) WriteMsg(
On a second thought, I don't feel very strongly about this one, and I might just remove it. When it comes to the actual migration logic PR, it's either we call this, or make this a helper function within forwarder itself. A nice thing about removing this is that we can treat interceptors as readers entirely.
There was a problem hiding this comment.
LGTM, nice simplification 💯
On a second thought, I don't feel very strongly about this one, and I might just remove it. When it comes to the actual migration logic PR, it's either we call this, or make this a helper function within forwarder itself. A nice thing about removing this is that we can treat interceptors as readers entirely.
+1 for removing WriteMsg. IMO methods that do not access instance state or implement interfaces are a code smell.
| // NewFrontendInterceptor creates a FrontendInterceptor. If bufSize is smaller | ||
| // than 5 bytes, the defaults (8K) will be used. | ||
| // | ||
| // NOTE: For future improvement, we can use the options pattern here if there's |
There was a problem hiding this comment.
nit: I recommend removing the bufSize argument. We do not need it and it would be a backwards compatible change to add it as a varg option in the future.
Previously, interceptors connect a source to a destination. This can be awkward during connection migration because we will now need to update the destination for an existing interceptor. Adding an UpdateWriter (or similar) does not seem right. This PR changes how interceptors work. Instead of connecting one end to another, interceptors are now one sided only. When attempting a write through ForwardMsg, callers will need to pass in a destination of type io.Writer, resulting in a much cleaner API when it comes to connection migration. We will also remove WriteMsg in this commit. This makes interceptors purely readers wrapping net.Conn objects. Since interceptors no longer have destinations, keeping the closed field is not very useful, so this commit removes that as well. A warning has been added to ensure that callers do not reuse interceptors or destinations whenever a ReadMsg or ForwardMsg call returns an error. Release note: None
… error Previously, we had an errPanicWriter struct that panics whenever a Write call was made. This is used because Receive on the pgproto3 backend and frontend instances must not call Write. However, panics are not ideal, so this commit replaces that with a regular writer that just returns an error when Write is called. Release note: None
Previously, we returned an error whenever callers attempt to create interceptors with a small buffer size. This case is very uncommon, and the API can be awkward since we now need to handle the error case. To address that, this commit updates the interceptor's behavior such that we default to an 8K buffer whenever a buffer size smaller than 5 bytes is used. Since sqlproxy is the only user, this seems to be a reasonable tradeoff. At the same time, we also make the specialized interceptors to default to a buffer size of 8K bytes. Release note: None
jaylim-crl
force-pushed
the
jay/cleanup-interceptor-api
branch
from
db4533b to
6dce0bf
Compare
There was a problem hiding this comment.
TFTR!
IMO methods that do not access instance state or implement interfaces are a code smell.
+1. Done.
Reviewable status:
pkg/ccl/sqlproxyccl/interceptor/frontend_interceptor.go, line 24 at r4 (raw file):
Previously, JeffSwenson (Jeff Swenson) wrote…
nit: I recommend removing the bufSize argument. We do not need it and it would be a backwards compatible change to add it as a varg option in the future.
Done. I only removed the ones on the specialized interceptors. Left the field on pgInterceptor as-is.
|
bors r=JeffSwenson |
|
Build succeeded: |
Informs #76000.
ccl/sqlproxyccl: update ForwardMsg to take in an io.Writer
Previously, interceptors connect a source to a destination. This can be
awkward during connection migration because we will now need to update the
destination for an existing interceptor. Adding an UpdateWriter (or similar)
does not seem right.
This PR changes how interceptors work. Instead of connecting one end to
another, interceptors are now one sided only. When attempting a write through
ForwardMsg, callers will need to pass in a destination of type io.Writer,
resulting in a much cleaner API when it comes to connection migration. We
will also remove WriteMsg in this commit. This makes interceptors purely
readers wrapping net.Conn objects.
Since interceptors no longer have destinations, keeping the closed field is
not very useful, so this commit removes that as well. A warning has been
added to ensure that callers do not reuse interceptors or destinations whenever
a ReadMsg or ForwardMsg call returns an error.
ccl/sqlproxyccl: replace errPanicWriter with a writer that returns an error
Previously, we had an errPanicWriter struct that panics whenever a Write
call was made. This is used because Receive on the pgproto3 backend and
frontend instances must not call Write. However, panics are not ideal, so
this commit replaces that with a regular writer that just returns an error
when Write is called.
ccl/sqlproxyccl: use a default buffer size of 8K within the interceptors
Previously, we returned an error whenever callers attempt to create
interceptors with a small buffer size. This case is very uncommon, and the API
can be awkward since we now need to handle the error case. To address that,
this commit updates the interceptor's behavior such that we default to an 8K
buffer whenever a buffer size smaller than 5 bytes is used. Since sqlproxy is
the only user, this seems to be a reasonable tradeoff.
At the same time, we also make the specialized interceptors to default to
a buffer size of 8K bytes.
Release note: None