Feature/receive server query

[yuzifeng1984]

Update protocol to support receiving query id from server

[yuzifeng1984]

@chenziliang @ye11ow

I merged proton PR to encode server node id into query UUID in https://github.com/timeplus-io/proton-enterprise/pull/5460

This golang driver PR is to receive the server generated query ID and return it to the caller. In order to use the query id (eg. kill query/get pipeline in cluster), caller need provide callback function in QueryOption as below. Please help to review the PR.

func WithReceiveQueryID(fn func(string)) QueryOption {
	return func(o *QueryOptions) error {
		o.events.receiveQueryID = fn
		return nil
	}
}

Also refer to ut code.
https://github.com/timeplus-io/proton-go-driver/pull/70/files#diff-9f2a2381e0cc03e4ae97084cd3fb6b580de684a4e63963f843ff99078e5b5048

[ye11ow]

@yuzifeng1984 I'm purely speaking from the user side (the user of proton-go-driver):
The current approach looks weird to me. For the TestServerGenerateQueryID I'd expect something like

ctx = proton.Context(context.Background())
conn, err = proton.Open(...)

serverGenearatedID, _, err := conn.Query(ctx, "SELECT 123")

For the TestClientGenerateQueryID, I'm not quite sure why I have to pass both proton.WithQueryID(id) and proton.WithReceiveQueryID. I'd expect the client code remains unchanged

[yuzifeng1984]

@yuzifeng1984 I'm purely speaking from the user side (the user of proton-go-driver): The current approach looks weird to me. For the TestServerGenerateQueryID I'd expect something like

ctx = proton.Context(context.Background())
conn, err = proton.Open(...)

serverGenearatedID, _, err := conn.Query(ctx, "SELECT 123")

For the TestClientGenerateQueryID, I'm not quite sure why I have to pass both proton.WithQueryID(id) and proton.WithReceiveQueryID. I'd expect the client code remains unchanged

@ye11ow The unit test case is just for testing purpose.

In real case, user could either give an ID or leave it unset to allow server generate one with the node id in sending query.

1. When user gives an ID by proton.WithQueryID(id). Proton will not generate one with the node id information. User does not need to set proton.WithReceiveQueryID callback in this case since the same ID will be returned. But the user generated query ID will have the kill-query issue in cluster.

2. When user does not have proton.WithQueryID(id). Proton will generate the ID. User need to set proton.WithReceiveQueryID to get the server generated ID if user wants to use the ID information for further queries.

[ye11ow]

@yuzifeng1984 I see.
Yeah, I think 1. makes sense.
For two, I got the idea but I think from API design perspective, the WithReceiveQueryID approach is not ideal. This adds quite a few code to client. Just like your example, client needs to be called like this:

idCh = make(chan string, 1)
ctx  = proton.Context(context.Background(), proton.WithReceiveQueryID(func(id string) {
  idCh <- id
  close(idCh)
}))
conn, _ = proton.Open(...)
_, err := conn.Query(ctx, "SELECT 123")
id := <-idCh

// do whatever with id

I'd prefer

ctx  = proton.Context(context.Background())
conn, _ = proton.Open(...)
id, _, err := conn.Query(ctx, "SELECT 123")

// do whatever with id

There are few issues with the current approach

1. More boilerplate code
2. I have no idea when WithReceiveQueryID will be called. Is that immediately or after a few seconds? Shall I start the goroutine to read the data and use another goroutine to wait for the ID?

[yuzifeng1984]

@ye11ow Yes. the callback method is a little ugly. While changing Query method interface to return query id seems even worse, it will break every existing user code. Also, as the new signature is not consistent with golang std lib, a lot of under layer code need be changed.

For the callback itself, it will be called synchronously before Query method return. There is some boilerplate code which you may not want to add in neutron. I am thinking if proton-go-driver to provide a new API to wrap these; such as QueryReturnID(context, query) -> (Rows, ID, Error). But you still need to change neutron to call the new method in the places where you want server to generate query ID. What do you think?

[yuzifeng1984]

@ye11ow I checked the neutron code. It looks the query is through the sql.DB in golang lib whose methods can not be changed. We have no choice than the WithReceiveQueryID approach to return the query ID.

https://github.com/timeplus-io/neutron/blob/develop/internal/proton/driver.go#L19

[ye11ow]

@yuzifeng1984 You are right, I almost forgot this: we are using std golang SQL interface so you cannot really change the signature.

Yeah, then we don't even have an option here. Have to go with the WithReceiveQueryID option.

[chenziliang]

Is it safe to do following since string is immutable ? my gut feeling is either using ctx or callback for customization. Callback is more straight forward / explicit in my view

var query_id string;

ctx  = proton.Context(context.Background(), proton.WithReceiveQueryID(func(id string) {
    query_id = id;
}))

conn, _ = proton.Open(...)
_, err := conn.Query(ctx, "SELECT 123")

// do whatever query_id

[chenziliang]

is it possible to mutate ctx to carry back the query_id if a callback is not ideal ?

_, err := conn.Query(ctx, "SELECT 123")

[chenziliang]

We don't think we need this ?

[yuzifeng1984]

This is cherry picked from click house driver to support protocol 54459. since we bump the client protocol to 54461 for uuid.

[yuzifeng1984]

Is it safe to do following since string is immutable ? my gut feeling is either using ctx or callback for customization. Callback is more straight forward / explicit in my view

var query_id string;

ctx  = proton.Context(context.Background(), proton.WithReceiveQueryID(func(id string) {
    query_id = id;
}))

conn, _ = proton.Open(...)
_, err := conn.Query(ctx, "SELECT 123")

// do whatever query_id

@chenziliang @ye11ow
Context is immutable and seems not right way to pass back data. I simplify the test case by directly assigning local string variable since the callback is invoked before Query method return.

https://github.com/timeplus-io/proton-go-driver/pull/70/files#diff-9f2a2381e0cc03e4ae97084cd3fb6b580de684a4e63963f843ff99078e5b5048R14

[chenziliang]

Is it safe to do following since string is immutable ? my gut feeling is either using ctx or callback for customization. Callback is more straight forward / explicit in my view

var query_id string;

ctx  = proton.Context(context.Background(), proton.WithReceiveQueryID(func(id string) {
    query_id = id;
}))

conn, _ = proton.Open(...)
_, err := conn.Query(ctx, "SELECT 123")

// do whatever query_id

@chenziliang @ye11ow Context is immutable and seems not right way to pass back data. I simplify the test case by directly assigning local string variable since the callback is invoked before Query method return.

https://github.com/timeplus-io/proton-go-driver/pull/70/files#diff-9f2a2381e0cc03e4ae97084cd3fb6b580de684a4e63963f843ff99078e5b5048R14

This is much easier and totally makes sense to me