Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Flight] Flight error status for quota exceeded #35888

Closed
chairmank opened this issue Jun 2, 2023 · 5 comments
Closed

[Flight] Flight error status for quota exceeded #35888

chairmank opened this issue Jun 2, 2023 · 5 comments
Assignees
@janosik47
Labels
Component: FlightRPC Type: enhancement
Milestone
17.0.0

Comments

@chairmank
Copy link

Describe the enhancement requested

I would like a Flight error status that communicates that a call was rejected because the client exceeded a usage quota. I do not see such an error status in the code or in the documentation. It is semantically distinct from the Unauthorized or Unavailable statuses.

For Flight over gRPC transport, this error status could map to the RESOURCE_EXHAUSTED gRPC status code (for example, see Google APIs).

This other issue appears to be relevant to my feature request: #34544

Component(s)

FlightRPC
@lidavidm
Copy link
Member

lidavidm commented Jun 2, 2023

Yes, it might be good to just adopt the rest of the gRPC codes while we're at it...

@lidavidm lidavidm mentioned this issue Jul 17, 2023
Closed
@janosik47
Copy link
Contributor

janosik47 commented Apr 30, 2024 •
edited
Loading

Any chance to have this fixed? It feels more like a bug rather than an enhancement.

Silently replacing the RESOURCE_EXHAUSTED with INVALID_ARGUMENT changes the meaning of the message send to the other party of the communication.
One may decide not to do any retry in case of invalid argument (chances are slim that re-sending the same request will result with any other/better response) where in case of a resource/quota exceeded an exponential backoff and retry is a standard practice.

janosik47 added a commit to janosik47/arrow that referenced this issue May 2, 2024
@janosik47
apacheGH-35888: [Java] Add FlightStatusCode.RESOURCE_EXHAUSTED
dae7b39 
Support for apache#35888

Add the FlightStatusCode.RESOURCE_EXHAUSTED code and make sure is translated from/to the gRPC Status.RESOURCE_EXHAUSTED
@janosik47 janosik47 mentioned this issue May 2, 2024
Merged
lidavidm pushed a commit that referenced this issue May 3, 2024
@janosik47
GH-35888: [Java] Add FlightStatusCode.RESOURCE_EXHAUSTED (#41508)
0d8b379 
### Rationale for this change

Related to #35888

Currently the gRPC Status.RESOURCE_EXHAUSTED exception/code is translated by the Java FlightServer into FlightStatusCode.INVALID_ARGUMENT and thrown to the client as gRPC INVALID_ARGUMENT exception.

That may mislead the other party as the INVALID_ARGUMENT indicates an input parameters problem where in reality the backed server intention was rather 'back off and try later'.

### What changes are included in this PR?

Add the FlightStatusCode.RESOURCE_EXHAUSTED code and make sure is translated from/to the gRPC Status.RESOURCE_EXHAUSTED

### Are these changes tested?

Unit tests included to validate the RESOURCE_EXHAUSTED translation  between flight and grpc codes.

### Are there any user-facing changes?

No.

Users may start seeing RESOURCE_EXHAUSTED instead of INVALID_ARGUMENT code. In both cases this is an exception seen on the client side so I am considering this as a _not breaking change to any public API_.

Although, may have an influence in the client side flows if one decided to react conditionally on exception status code.

* GitHub Issue: #35888

Authored-by: Jacek Stania <janosik47@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
@lidavidm lidavidm added this to the 17.0.0 milestone May 3, 2024
@lidavidm
Copy link
Member

lidavidm commented May 3, 2024

Issue resolved by pull request 41508
#41508

@lidavidm lidavidm closed this as completed May 3, 2024
vibhatha pushed a commit to vibhatha/arrow that referenced this issue May 25, 2024
@janosik47 @vibhatha
apacheGH-35888: [Java] Add FlightStatusCode.RESOURCE_EXHAUSTED (apach…
72c9d3d 
…e#41508)

### Rationale for this change

Related to apache#35888

Currently the gRPC Status.RESOURCE_EXHAUSTED exception/code is translated by the Java FlightServer into FlightStatusCode.INVALID_ARGUMENT and thrown to the client as gRPC INVALID_ARGUMENT exception.

That may mislead the other party as the INVALID_ARGUMENT indicates an input parameters problem where in reality the backed server intention was rather 'back off and try later'.

### What changes are included in this PR?

Add the FlightStatusCode.RESOURCE_EXHAUSTED code and make sure is translated from/to the gRPC Status.RESOURCE_EXHAUSTED

### Are these changes tested?

Unit tests included to validate the RESOURCE_EXHAUSTED translation  between flight and grpc codes.

### Are there any user-facing changes?

No.

Users may start seeing RESOURCE_EXHAUSTED instead of INVALID_ARGUMENT code. In both cases this is an exception seen on the client side so I am considering this as a _not breaking change to any public API_.

Although, may have an influence in the client side flows if one decided to react conditionally on exception status code.

* GitHub Issue: apache#35888

Authored-by: Jacek Stania <janosik47@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
@janosik47
Copy link
Contributor

Any chance this can be included in a next release please?

@lidavidm
Copy link
Member

Releases happen on a regular schedule. https://lists.apache.org/thread/hob7ozny5f49mgc5hpxz9qlyytp8w38v

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@janosik47 janosik47

Labels
Component: FlightRPC Type: enhancement
Projects
None yet
Milestone
17.0.0
Development

No branches or pull requests
3 participants
@lidavidm @chairmank @janosik47