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.

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

[YUNIKORN-2249] Add Accept-Encoding header when fetch queue application API #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

targetoee
Copy link
Contributor

What is this PR for?

When the API response size is large, it takes a long time to transfer data.
This PR adds a 'Accept-Encoding' request header. If the client (web side) indicates gzip encoding, the scheduler will compress the data before sending it back. So it can reduce the data size and transfer time.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

N/A

What is the Jira issue?

YUNIKORN-2249

How should this be tested?

local build

Screenshots (if appropriate)

N/A

Questions:

N/A

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8325d7c) 66.66% compared to head (5ea6626) 66.66%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #158   +/-   ##
=======================================
  Coverage   66.66%   66.66%           
=======================================
  Files           1        1           
  Lines          30       30           
=======================================
  Hits           20       20           
  Misses          7        7           
  Partials        3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -82,8 +82,10 @@ export class SchedulerService {

fetchAppList(partitionName: string, queueName: string): Observable<AppInfo[]> {
const appsUrl = `${this.envConfig.getSchedulerWebAddress()}/ws/v1/partition/${partitionName}/queue/${queueName}/applications`;
const headers = new HttpHeaders();
headers.set('Accept-Encoding', 'gzip')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work with scheduler without apache/yunikorn-core#757 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

it should just be ignored if the server does not support it. Fallback will always be identity transform (i.e. no compression)

@wilfred-s
Copy link
Contributor

More general: should we not be setting this on all requests that we do? I can see requests also get large when we do get a large number of nodes etc.
If the server does not support it or decides not to zip (i.e. below the MTU size) it should all be OK even if set.

@targetoee
Copy link
Contributor Author

Most browsers automatically include 'Accept-Encoding: gzip, deflate' in the request header, so adding it is redundant. Additionally, we may not need to compress or decompress all the data on web at present. Simply providing a compression option for the user may be enough.
Keep things as before is one choice. Or any other suggestion?

@chia7712
Copy link
Contributor

Personally, gzip is good enough

@craigcondit
Copy link
Contributor

craigcondit commented Mar 21, 2024

Most browsers automatically include 'Accept-Encoding: gzip, deflate' in the request header, so adding it is redundant. Additionally, we may not need to compress or decompress all the data on web at present. Simply providing a compression option for the user may be enough. Keep things as before is one choice. Or any other suggestion?

Except not all clients are browsers (and the header is not added by default to things like API calls from javascript in many frameworks).

The purpose of the Accept-Encoding header is to allow content negotiation. If the header is not present on a request, the server MUST interpret it as if Accept-Encoding: identity were sent -- the identity encoding is effectively a no-op. Clients are required to accept unencoded responses; they are not required to accept any form of compression at all. The purpose of the header is to inform the server of the client's capabilities, so that the server may choose a compatible encoding. The server MUST choose from one of the available encodings, or fall back to identity if no common encoding exists. Since we are adding gzip support to our server, we simply need to test if the client supports it; if so we use gzip otherwise we don't compress. We MUST NOT send an error in any case, and we MUST NOT send gzip encoded data to a client which has not requested it explicitly.

By the way -- I'm not trying to be obnoxious with the MUST and MUST NOT terminology; those are taken directly from the language used by the RFCs that define how HTTP is supposed to work.

https://datatracker.ietf.org/doc/html/rfc9110#name-accept-encoding

Note that some of the tests given that RFC aren't completely practical; For example, "If no Accept-Encoding header field is in the request, any content coding is considered acceptable by the user agent" -- this isn't really ever true in practice. If I want to return a resource encoded as my-private-encoding it's silly to think that any client would be able to accept that. In practice, it's far safer to assume identity encoding only so that clients aren't confronted with unexpected encodings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants