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

[YUNIKORN-2249] Add compression option to getQueueApplication API #757

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

[YUNIKORN-2249] Add compression option to getQueueApplication API #757

Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9ef7968
add compression part
targetoee Dec 12, 2023
85dd3de
add compression part
targetoee Dec 12, 2023
b5e2651
modify compress function and add test
targetoee Mar 8, 2024
e7ea5bc
Merge branch 'master' into YUNIKORN-2249
targetoee Mar 9, 2024
7238626
fix lint
targetoee Mar 10, 2024
ce40af6
pull origin branch
targetoee Mar 10, 2024
995cb97
close writer and check supported comp type
targetoee Mar 12, 2024
4cb1d9c
skip compress step if the data size is small
targetoee Mar 12, 2024
40b9649
correct header setting
targetoee Mar 12, 2024
4f57568
modify compress & checkheader function
targetoee Mar 17, 2024
4e7228a
remove unused testcase
targetoee Mar 19, 2024
2205902
unused import
targetoee Mar 19, 2024
b811993
draft testcase
targetoee Mar 19, 2024
8a6f9cf
handle compression in unified manner
targetoee Apr 5, 2024
53c8827
Merge branch 'master' into YUNIKORN-2249
targetoee Apr 5, 2024
1c4a0e1
fix test
targetoee Apr 5, 2024
41c0570
add compression test case
targetoee Apr 8, 2024
ca18e79
add webservice test
targetoee Apr 27, 2024
0a6c931
adjust error message and close connection immediately
targetoee Apr 27, 2024
3f806ba
adjust function name and message
targetoee Apr 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions pkg/webservice/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
package webservice

import (
"bytes"
"compress/gzip"
"encoding/json"
"fmt"
"io"
Expand Down Expand Up @@ -614,6 +616,33 @@
appsDao = append(appsDao, getApplicationDAO(app))
}

if strings.Contains(r.Header.Get("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.

Could we have test for gzip header?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try to make it modular? We already thinking of doing it in two more places. We should be able to use the method wherever required.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Test is missing
  2. Pls extract all headers to a const section, it's better for maintainability. We have string literals all over the place, eg. writeHeaders(). I don't think we need to import a separate library for this, but at least have a single place where all of them are defined. Also do the same for MIME types.

Copy link
Contributor

Choose a reason for hiding this comment

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

By modular I mean create two functions. One that tests for the header content see other review point and one that does all the compress work and takes two parameters and returns nothing. We can use that compress function anywhere.

func checkHeader(h http.Header, key, value string) bool
func compress(w http.ResponseWriter, v any)

Need to file a follow up jira when we add this to add support for all REST endpoints to support compressed data...

Copy link
Contributor

Choose a reason for hiding this comment

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

Testing for gzip as we do now is broken. The header is an array of strings and get only returns the first value. So if I call the URL from the browser it depends on the browser if gzip is the first in the list. We should use this:

func checkHeader(h map[string][]string, key, value string) bool {
	values := h.Values(key)
	for _, v := range values {
		if v == value {
			return true
		}
	}
	return false
}

w.Header().Set("Content-Encoding", "gzip")

response, err := json.Marshal(appsDao)
if err != nil {
buildJSONErrorResponse(w, err.Error(), http.StatusBadRequest)
return
}

Check warning on line 626 in pkg/webservice/handlers.go

View check run for this annotation

Codecov / codecov/patch

pkg/webservice/handlers.go#L620-L626 

Added lines #L620 - L626 were not covered by tests

var comp bytes.Buffer
writer := gzip.NewWriter(&comp)
_, err = writer.Write(response)
if err != nil {
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
return
}
err = writer.Close()
if err != nil {
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
return
}
if _, err = w.Write(comp.Bytes()); err != nil {
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
}
return

Check warning on line 643 in pkg/webservice/handlers.go

View check run for this annotation

Codecov / codecov/patch

pkg/webservice/handlers.go#L628-L643 

Added lines #L628 - L643 were not covered by tests
}

if err := json.NewEncoder(w).Encode(appsDao); err != nil {
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
}
Expand Down