Skip to content

Conversation

@amaliujia
Copy link
Contributor

@amaliujia amaliujia commented Nov 11, 2022

What changes were proposed in this pull request?

This PRs introduces a ClientType into Connect proto that can be included into Request to indicate the client type (e.g. Python client, Scala client, etc.)

Why are the changes needed?

Better understand requests by the server.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UT

@amaliujia
Copy link
Contributor Author

amaliujia commented Nov 11, 2022

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid changing this all the time a new client comes along, this should most likely be a string.

Suggested change
ClientType client_type = 4;
optional string client_type = 4;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// From which client this request was sent from.
// Provides optional information about the client sending the request. This field
// can be used for language or version specific information and is only intended for
// logging purposes and will not be interpreted by the server.

Copy link
Contributor

@grundprinzip grundprinzip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a specific use case in mind?

@amaliujia
Copy link
Contributor Author

amaliujia commented Nov 11, 2022

@grundprinzip I actually want to log to different entries for the usage from different clients.

The perfect logging to me is

x jobs submitted through proto (which literally is the entire connect usage)
then breakdown
y from client A
z from client B.

Maybe, just say maybe, we will see user self-implemented clients by this field (or if this field is not set we will also know).

Also I would still imagine that we need send through client side language spec (language type, version, etc.) which you already mentioned in the suggestion. Maybe eventually we need a message named client metadata. But right now only which client is added to proto for this metadata.

We can see if we need client side language spec eventually (when we deal with UDF probably)

@grundprinzip
Copy link
Contributor

All this in mind it still makes more sense to keep this as an optional string field instead of an enum.

Copy link
Contributor

@grundprinzip grundprinzip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the type from enum to string.

@amaliujia
Copy link
Contributor Author

Yeah let me take a look.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@amaliujia amaliujia changed the title [SPARK-41115][CONNECT] Add ClientType Enum to proto to indicate which client sends a request [SPARK-41115][CONNECT] Add ClientType to proto to indicate which client sends a request Nov 13, 2022
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this do the same thing as user_context.extensions ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is purely a string and not an extension type based on Any

@grundprinzip
Copy link
Contributor

@amaliujia can you please update the pr description to remove the enum part.

@HyukjinKwon
Copy link
Member

Merged to master.

SandishKumarHN pushed a commit to SandishKumarHN/spark that referenced this pull request Dec 12, 2022
…nt sends a request

### What changes were proposed in this pull request?

This PRs introduces a `ClientType` into Connect proto that can be included into Request to indicate the client type (e.g. Python client, Scala client, etc.)

### Why are the changes needed?

Better understand requests by the server.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing UT

Closes apache#38630 from amaliujia/client-type.

Authored-by: Rui Wang <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
beliefer pushed a commit to beliefer/spark that referenced this pull request Dec 15, 2022
…nt sends a request

### What changes were proposed in this pull request?

This PRs introduces a `ClientType` into Connect proto that can be included into Request to indicate the client type (e.g. Python client, Scala client, etc.)

### Why are the changes needed?

Better understand requests by the server.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing UT

Closes apache#38630 from amaliujia/client-type.

Authored-by: Rui Wang <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
beliefer pushed a commit to beliefer/spark that referenced this pull request Dec 18, 2022
…nt sends a request

### What changes were proposed in this pull request?

This PRs introduces a `ClientType` into Connect proto that can be included into Request to indicate the client type (e.g. Python client, Scala client, etc.)

### Why are the changes needed?

Better understand requests by the server.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing UT

Closes apache#38630 from amaliujia/client-type.

Authored-by: Rui Wang <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants