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

WIP: Parallel Decomp - Global pool in separate class and... #3556

Open
wants to merge 8 commits into
base: devel
Choose a base branch
from

Conversation

HereThereBeDragons
Copy link
Contributor

@HereThereBeDragons HereThereBeDragons commented Mar 27, 2024

CVMFS params to adjust memory footprint

TODO:

  • Adjust Clone() for external downloadmanager. right now a Clone() does nothing and will disable this feature.
  • Add integration test. Right now nothing tests this feature --> unittest

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/4343/

@cernvm-bot
Copy link
Collaborator

@HereThereBeDragons HereThereBeDragons marked this pull request as draft March 27, 2024 13:51
@cernvm-bot
Copy link
Collaborator

linter finished with errors:

Done processing mount/mount.cvmftest/unittests/t_download.cc:571:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/unittests/t_download.cc:609:  Missing space after ,  [whitespace/comma] [3]
test/unittests/t_download.cc:619:  Missing space after ,  [whitespace/comma] [3]

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/4344/

@cernvm-bot
Copy link
Collaborator

@HereThereBeDragons
Copy link
Contributor Author

cloudtest looks good. the 2 commits since then did not touch any code logic --> no new cloudtest need

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

linter finished with errors:

Done processingcvmfs/network/download.cc:1445:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
cvmfs/network/download.h:266:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/4348/

@cernvm-bot
Copy link
Collaborator

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/4349/

@cernvm-bot
Copy link
Collaborator

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/4350/

@cernvm-bot
Copy link
Collaborator

} else {
const int64_t written = info->sink()->Write(ptr, num_bytes);
if (written < 0 || static_cast<uint64_t>(written) != num_bytes) {
PANIC(kLogStderr | kLogDebug, "(id %" PRId64 ") "
Copy link
Contributor

Choose a reason for hiding this comment

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

In the CallbackCurlData() this is a plain LogCvmfs() statement, but here its a PANIC(). Is there a reason why? If possible, it might make sense to refactor these section into a single helper if they are identical. Also, if we cant write to the sync, we should stop the download, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching this!
yes also the download should be stopped

@cernvm-bot
Copy link
Collaborator

linter finished with errors:

Done processingcvmfs/network/download.cc:2143:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
cvmfs/network/download.h:268:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/4351/

@cernvm-bot
Copy link
Collaborator

@HereThereBeDragons
Copy link
Contributor Author

last thing what is missing is the rebase onto devel

@HereThereBeDragons HereThereBeDragons force-pushed the dwnld_mgr_parallel_decomp_3493_v2_static_global_pool branch from 28ff63b to 0e2c64c Compare April 15, 2024 11:37
@HereThereBeDragons
Copy link
Contributor Author

@cernvm-bot cloudtest

@cernvm-bot
Copy link
Collaborator

building for cloudtests finished: SUCCESS
https://lcgapp-services.cern.ch/cvmfs-jenkins/job/CvmfsFullBuildDocker/4374/

@cernvm-bot
Copy link
Collaborator

@HereThereBeDragons HereThereBeDragons requested review from rezan and vvolkl and removed request for rezan April 15, 2024 14:08
@HereThereBeDragons
Copy link
Contributor Author

this pr is now ready to review
(needs some more performance testing)

it includes the fix for #3563

@HereThereBeDragons
Copy link
Contributor Author

Fixes #3493

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

3 participants