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

persist log level to reinitialize logger for eos tus #923

Closed
wants to merge 1 commit into from

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Jul 1, 2020

makes log lines for actual eos / xrdcopy cmds reappear ;-)

@butonic butonic requested a review from labkode as a code owner July 1, 2020 14:19
Copy link
Member

@labkode labkode left a comment

Choose a reason for hiding this comment

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

This approach looks wrong. The logger should be already present in the context given to the function

@butonic
Copy link
Contributor Author

butonic commented Jul 1, 2020

The tus hander disconnects the internal handling of the storage write by creating a new context. The goal is to let the server write as many bytes as possible. I raised this as an issue upstream. So it is by design.

That being said I think it helps debugging because admins can lookup the id and if needed increase the loglevel for a specific upload. We could even add a service or an api to the dataprovider that can manage ongoing uploads.

@butonic butonic force-pushed the build-upload-context branch from 54fd5d9 to 2168d9c Compare July 1, 2020 18:52
@labkode
Copy link
Member

labkode commented Jul 2, 2020

@butonic please create an issue to remember about that upstream fix for later cleanup.
Why this fix is not needed for the localfs?

I've started refactoring the tus handler and basically the way data uploads/downloads are working here: #828

@butonic
Copy link
Contributor Author

butonic commented Jul 13, 2022

we should pass a logger / context down when initializing the service

@butonic butonic closed this Jul 13, 2022
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.

2 participants