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

chore(graphql): cleanup, refactor, async execution #1202

Merged
merged 13 commits into from
Nov 8, 2022

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Nov 6, 2022

Fixes #1201
Related to #1214 / #1193
Depends on cryostatio/cryostat-web#611

@andrewazores andrewazores added the chore Refactor, rename, cleanup, etc. label Nov 6, 2022
@mergify mergify bot added the safe-to-test label Nov 6, 2022
@andrewazores andrewazores marked this pull request as ready for review November 6, 2022 22:54
@andrewazores andrewazores requested a review from maxcao13 November 6, 2022 22:54
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1202-351a12ae54bd45d6f901102b2aeb2d073b98accb sh smoketest.sh

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1202-f6ffaedd36f42e5c6ffcd8031bc842fa2b994ed2 sh smoketest.sh

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1202-2f6d9e3a5f3ecac4c7ab71168ae8c117544bcacb sh smoketest.sh

Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

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

I get

Request failed (500 Internal Server Error)

Internal Server Error caused by JsonParseException: Unrecognized token 'query': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false') at [Source: (io.netty.buffer.ByteBufInputStream); line: 2, column: 13]

When I navigate to the Recording tab.

@andrewazores
Copy link
Member Author

Oh, I guess this does actually need cryostatio/cryostat-web#611 in that case. I worked on them together but it seemed they should work independently. The request being sent has the raw GraphQL query { ... } string as the POST request body, which the handler doesn't seem to know what to do with.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

This PR/issue depends on:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1202-84b45739a06c04ddea57fa64ab1d824405257b0c sh smoketest.sh

@andrewazores
Copy link
Member Author

Most recent commit touches up the HTTP request logging a little.

INFO: 127.0.0.1 - - [Tue, 8 Nov 2022 00:02:31 GMT] 46ms "GET /api/beta/fs/recordings HTTP/1.1" 200 354 bytes "http://localhost:9000/" "Mozilla/5.0 (X11; Linux x86_64; rv:106.0) Gecko/20100101 Firefox/106.0"

The 46ms is new. Our old custom format built-in to the WebServer class included that detail and it was lost when I switched over to extracting it to a separate Handler and delegating to a Vert.x default implementation. That default implementation also includes the content-length of the response in raw bytes, and I've touched it up to use FileUtils.byteCountToDisplaySize from commons-io so that it's a bit more readable.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1202-f16b4d46ffc3ece61ccfceaad65baa974edef4b3 sh smoketest.sh

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1202-f04d75ef8f1304cf02c1c2777bca982f9e1e733c sh smoketest.sh

Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

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

Everything looks good and graphql does seem to perform better!

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1202-f993dbafd83fa796bee48af696d24badafbbbf5c sh smoketest.sh

@andrewazores andrewazores merged commit ac9d348 into cryostatio:main Nov 8, 2022
@andrewazores andrewazores deleted the async-graphql2 branch November 8, 2022 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. safe-to-test
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] GraphQL async execution
2 participants