Do not add new chunks to already-large splits. #7186
Conversation
|
A new Pull Request was created by @bbockelm (Brian Bockelman) for CMSSW_7_4_X. Do not add new chunks to already-large splits. It involves the following packages: Utilities/XrdAdaptor @cmsbuild, @Dr15Jones, @ktf, @nclopezo can you please review it and eventually sign? Thanks. |
|
@bbockelm Brian, can you comment as to why we need the change? |
|
The xrootd client can only handle vectors of size 1024. If the splitting algorithm results in a too-large vector of IO chunks, then later sanity check code will assert. See this example bug report: https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3413.html |
| throw ex; | ||
| } | ||
| if (req1.size() < 1000) {consumeChunkFront(front, tmp_iolist, req1, chunk1);} | ||
| if (req2.size() < 1000) {consumeChunkBack(front, tmp_iolist, req2, chunk2);} | ||
| } | ||
| std::sort(req1.begin(), req1.end(), [](const IOPosBuffer & left, const IOPosBuffer & right){return left.offset() < right.offset();}); | ||
| std::sort(req2.begin(), req2.end(), [](const IOPosBuffer & left, const IOPosBuffer & right){return left.offset() < right.offset();}); |
There was a problem hiding this comment.
FYI you can acutally reuse the same lambda by doing
auto compare = [](const IOPosBuffer & left, const IOPosBuffer & right){return left.offset() < right.offset();};
|
+1 |
|
please test |
|
The tests are being triggered in jenkins. |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes or unless it breaks tests. |
|
looks like this PR needs a rebase. |
With this, we refuse to add new entries to a chunk list that is already 1000 entries. If both splits are above 1000 entries, then an exception is thrown. As the upper layers of CMSSW guarantee this shouldn't happen, I feel throwing an exception is the correct behavior (compared to error recovery).
bbockelm
force-pushed
the
fix_large_split
branch
from
7a3f75b
to
2364d5b
Compare
|
Hi @davidlange6 - I think github got confused somehow; I couldn't find any merge conflicts. In desperation, I rebased on top of the latest of the CMSSW_7_4_X (sorry Chris!) and now github has a green light. |
cmsbuild
added
comparison-pending
and removed
comparison-pending
core-approved
fully-signed
tests-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes or unless it breaks tests. |
|
The tests are being triggered in jenkins. |
|
-1 runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15+MINIAODMCUP15/step3_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15+MINIAODMCUP15.log 25202.0 step3 runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15+MINIAODMCUP15/step3_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15+MINIAODMCUP15.log you can see the results of the tests here: |
cmsbuild
added
comparison-pending
core-approved
fully-signed
tests-rejected
and removed
comparison-pending
core-approved
fully-signed
tests-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (but tests are reportedly failing). |
|
please test (previous errors unrelated (and fixed now)) From: cmsbuild [notifications@github.com] This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (but tests are reportedly failing). — |
cmsbuild
added
comparison-pending
core-approved
fully-signed
tests-approved
and removed
comparison-pending
core-approved
fully-signed
tests-rejected
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (tests are also fine). This pull request will be automatically merged. |
Do not add new chunks to already-large splits.
With this, we refuse to add new entries to a chunk list that is
already 1000 entries. If both splits are above 1000 entries, then
an exception is thrown.
As the upper layers of CMSSW guarantee this shouldn't happen, I
feel throwing an exception is the correct behavior (compared to
error recovery).