Skip to content

Extending the CCDB API to optionally get the headers from first HTTP request #14709

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.

Sign up for GitHub

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

Jump to bottom
Merged
sawenzel merged 4 commits into from
Oct 29, 2025
Merged

Extending the CCDB API to optionally get the headers from first HTTP request #14709

sawenzel merged 4 commits into from
Oct 29, 2025

Conversation

@mrtineide
Copy link
Contributor

@mrtineide mrtineide commented Oct 1, 2025

Some background

There is some code that used the retrieveHeaders function right after getting a object from CCDB, sending one request for the object itself and then one HTTP HEAD request immediately after for the metadata/headers.

I was asked by @costing to change the O2/CCDB C++ API such we could return the headers in one call, enabling the use of just one call to CCDB, avoiding the extra network calls/retrieveHeaders.

This PR implements these changes.

New functionality
  • Extended the CcdbApi::retrieveBlob to take a optional header paramater to return metadata/headers
  • Extend the BasicCCDBmanager to cache and return headers/metadata if asked for

This does increase the memory used, but I guess it is negligible.

(I also added some unit tests to make myself feel more comfortable with my changes)

@mrtineide mrtineide requested review from a team, Barthelemy, costing and sawenzel as code owners October 1, 2025 09:04
@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0
async-2024-PbPb-apass1
async-2024-ppRef-apass1
async-2024-PbPb-apass2
async-2023-PbPb-apass5

@alibuild
Copy link
Collaborator

alibuild commented Oct 1, 2025

Error while checking build/O2/fullCI_slc9 for 9f2938b at 2025-10-01 12:56:

## sw/BUILD/o2checkcode-latest/log
--
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
/sw/SOURCES/O2/14709-slc9_x86-64/0/CCDB/test/testCcdbApiHeaders.cxx:43:42: error: statement should be inside braces [readability-braces-around-statements]
++ [[ 0 == 0 ]]
++ exit 1
--

Full log here.

@mrtineide mrtineide force-pushed the output-optional-headers-with-tests branch from 9f2938b to d7b08a5 Compare October 1, 2025 11:08
@alibuild
Copy link
Collaborator

alibuild commented Oct 1, 2025

Error while checking build/O2/fullCI_slc9 for d7b08a5 at 2025-10-01 16:13:

++ cp /sw/slc9_x86-64/O2/14709-slc9_x86-64-local1/compile_commands.json .
+++ python3 -c 'import json, os; print(os.path.commonpath([x["file"] for x in json.loads(open("compile_commands.json").read()) if "sw/BUILD" not in x["file"] and "G__" not in x["file"] and x["file"].endswith(".cxx")]))'
++ O2_SRC=/sw/SOURCES/O2/14709-slc9_x86-64/0
++ [[ -e /sw/SOURCES/O2/14709-slc9_x86-64/0/CMakeLists.txt ]]

Full log here.

@mrtineide
Copy link
Contributor Author

mrtineide commented Oct 1, 2025

I see some of the unit test that I made are flaky, I am able to get tests locally to fail randomly when running them many times.
I think it has to do with some race condition or similar parallel execution regarding BasicCCDBManager singleton.

I'll change/remove the test are being flaky, so they do not pop up in other unrelated CI runs. I believe it is unrelated to the changes in this PR.

@sawenzel
Copy link
Collaborator

sawenzel commented Oct 1, 2025

I see some of the unit test that I made are flaky, I am able to get tests locally to fail randomly when running them many times. I think it has to do with some race condition or similar parallel execution regarding BasicCCDBManager singleton.

I'll change/remove the test are being flaky, so they do not pop up in other unrelated CI runs. I believe it is unrelated to the changes in this PR.

I do not see how in a unit test there should be parallel execution and race conditions with BasicCCDBManager unless you talk to it in multiple threads.

@mrtineide mrtineide force-pushed the output-optional-headers-with-tests branch from d7b08a5 to 5f6a483 Compare October 1, 2025 15:26
sawenzel
sawenzel reviewed Oct 1, 2025
View reviewed changes
@alibuild alibuild mentioned this pull request Oct 2, 2025
Closed
@mrtineide mrtineide force-pushed the output-optional-headers-with-tests branch from 519d0fb to 5f58d2f Compare October 2, 2025 14:25
@mrtineide
Extend CCDB functions with optional headers
f4feef8
@mrtineide
Remove the header filtering & add more tests
21ef657
@mrtineide
Fix code snippets and enhance metadata retrieval documentation
2028c5b 
Updated code snippets in README for header retrieval and added optional parameter documentation details.
Also fixed some CI formatting errors.
@jezwilkinson jezwilkinson mentioned this pull request Oct 7, 2025
Open
@mrtineide mrtineide force-pushed the output-optional-headers-with-tests branch from 5f58d2f to 510f451 Compare October 7, 2025 15:17
@alibuild alibuild mentioned this pull request Oct 7, 2025
Closed
@mrtineide mrtineide force-pushed the output-optional-headers-with-tests branch from 510f451 to 60ede12 Compare October 7, 2025 15:20
@alibuild
Copy link
Collaborator

alibuild commented Oct 8, 2025

Error while checking build/O2/fullCI_slc9 for 60ede12 at 2025-10-08 03:40:

No log files found

Full log here.

@mrtineide
Change unit test with to have correct includes
2246830 
This fixes some tests that are fine on my desktop but
the subprocess aborts even when all tests are green in the CI.
The error is this: malloc_consolidate(): invalid chunk size
Was told this was a heap corruption. And changing the
includes fixes the error, not sure why.
Changed also to only use one instance of CCDB api
so we do not call CURL global cleanup out of order
with the BasicCCDBManger singelton. Just in case
that matters when running the test(s).
@mrtineide mrtineide force-pushed the output-optional-headers-with-tests branch from 60ede12 to 2246830 Compare October 8, 2025 15:47
@mrtineide
Copy link
Contributor Author

mrtineide commented Oct 9, 2025

I see some of the unit test that I made are flaky, I am able to get tests locally to fail randomly when running them many times. I think it has to do with some race condition or similar parallel execution regarding BasicCCDBManager singleton.
I'll change/remove the test are being flaky, so they do not pop up in other unrelated CI runs. I believe it is unrelated to the changes in this PR.

I do not see how in a unit test there should be parallel execution and race conditions with BasicCCDBManager unless you talk to it in multiple threads.

That was what I initially though was happening, but I was incorrect. But turns out I included the wrong header in the unit test. I should have used #include <boost/test/unit_test.hpp and not #include <boost/test/included/unit_test.hpp!

@mrtineide mrtineide requested a review from sawenzel October 9, 2025 11:20
@alibuild
Copy link
Collaborator

alibuild commented Oct 13, 2025 •
edited
Loading

Error while checking build/O2/fullCI_slc9 for 2246830 at 2025-10-28 20:20:

## sw/BUILD/O2Physics-latest/log
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
Error in cling::AutoLoadingVisitor::InsertIntoAutoLoadingState:
[0 more errors; see full log]

Full log here.

@jezwilkinson
Copy link

jezwilkinson commented Oct 29, 2025

Hi, is there any news about the review of this PR? It would be good to have the API changes in place so that we can close AliceO2Group/O2Physics#13265

@sawenzel sawenzel merged commit 80f2bea into AliceO2Group:dev Oct 29, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sawenzel sawenzel sawenzel approved these changes

@costing costing Awaiting requested review from costing costing is a code owner

@Barthelemy Barthelemy Awaiting requested review from Barthelemy Barthelemy is a code owner

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mrtineide @alibuild @sawenzel @jezwilkinson