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

truncate execution output in default view #1025

Merged
merged 9 commits into from Apr 4, 2022

Conversation

fopina
Copy link
Contributor

@fopina fopina commented Nov 9, 2021

maxBufSize = 256000 in the shell handler, 100 executions per job, 25M to download when listing executions (and added browser rendering time)

The idea of this PR is to add an API parameter to truncate execution output but leave default to not truncate and avoid API breaking changes.

Then update UI to load list truncated but allow individual executions to have output fully loaded.

test

Create job with shell command seq 1000000 (seq 1000000 | wc -c = 6888894)

curl 'http://localhost:8080/v1/jobs' \
     --data-raw '{"name":"test","schedule":"@manually","executor":"shell","executor_config":{"command":"seq 1000000"}}'

Run it 100+ times:

for i in $(seq 100); do curl 'http://localhost:8080/v1/jobs/test' -X POST; done

Notes

Some parts pending re-design as I'm a complete react ignorant:

  • This PR does not include the generated assets to allow for a cleaner review first
  • output_size was added to dataprovider though it doesn't look correct there - how to send extra query param (output_size) from the ReferenceManyField component without modifying dataprovider? Or modifying it in a more generic way that makes sense to other components/resources
  • SpecialOutputPanel hammered in to have the load full output button, any suggestion for better UI/code? also need(should?) hide button if output is not truncated
  • missing tests for the new api endpoint (left for after initial review/comments)

vcastellm
vcastellm previously approved these changes Jan 16, 2022
Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @fopina LGTM thanks!

dkron/api.go Outdated Show resolved Hide resolved
dkron/api.go Outdated Show resolved Hide resolved
dkron/api.go Outdated Show resolved Hide resolved
dkron/api.go Outdated Show resolved Hide resolved
ui/src/jobs/JobShow.tsx Outdated Show resolved Hide resolved
@yvanoers
Copy link
Collaborator

Forgot to mention in the review: could you add tests to the new features in api.go?

@fopina
Copy link
Contributor Author

fopina commented Jan 22, 2022

Forgot to mention in the review: could you add tests to the new features in api.go?

this is one of the original notes I left in the PR text :) I didn't write the tests as I wasn't sure if the API would remain as it, due to my other notes which are highlighting parts I don't like in the PR.
One of them was hiding the button to highlight when there is anything more (that you also mentioned) but I don't know how the syntax to have those conditions in react, need to revisit it (though I was waiting for specific hints :( )

But also had a note for the way output_size is handled currently as I'm sure there's a prettier way...

@yvanoers
Copy link
Collaborator

this is one of the original notes I left in the PR text :) I didn't write the tests as I wasn't sure if the API would remain as it, due to my other notes which are highlighting parts I don't like in the PR.

Ah, indeed. I missed that, sorry!
I think the API is fine like this. Regardless of how the front end needs to be built, the ability to limit the output in the executions call is a good improvement. And retrieving data for one execution is also fine - except that I think that endpoint should be called .../executions/:execution (plural), come to think of it.

One of them was hiding the button to highlight when there is anything more (that you also mentioned) but I don't know how the syntax to have those conditions in react, need to revisit it (though I was waiting for specific hints :( )

I can't provide any hints on doing this off the top of my head, I would need to look into that just like you.

dkron/api.go Outdated Show resolved Hide resolved
fopina and others added 2 commits January 27, 2022 00:42
@fopina
Copy link
Contributor Author

fopina commented Jan 27, 2022

addressed all reviews and also included the test 🎉

@fopina fopina requested a review from yvanoers January 29, 2022 18:02
@fopinappb
Copy link

@yvanoers any chance this can be merged?

@yvanoers
Copy link
Collaborator

Have seen your comment - will get back to you.

@fopina
Copy link
Contributor Author

fopina commented Mar 24, 2022

@yvanoers :)

dkron/api.go Show resolved Hide resolved
ui/src/jobs/JobShow.tsx Outdated Show resolved Hide resolved
@yvanoers
Copy link
Collaborator

yvanoers commented Mar 24, 2022

@yvanoers :)

Yes, sorry, have a lot on my plate atm.
Anyway, nice work! Couple of findings still, but those should not be a big issue.

@fopina
Copy link
Contributor Author

fopina commented Mar 25, 2022

Yes, sorry, have a lot on my plate atm. Anyway, nice work! Couple of findings still, but those should not be a big issue.

no worries, I ping just to remind not to complain.

made commits to address the two comments, let me know if you agree

Copy link
Collaborator

@yvanoers yvanoers left a comment

Choose a reason for hiding this comment

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

Stamp of approval!

Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@vcastellm vcastellm merged commit 4dda46b into distribworks:master Apr 4, 2022
@fopina fopina deleted the truncate_execution_output branch January 22, 2023 22:44
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.

None yet

4 participants