Skip to content

Commit 21ef657

Browse files
mrtineidemrtineide
committed
Remove the header filtering & add more tests
1 parent f4feef8 commit 21ef657

File tree

3 files changed

+172
-154
lines changed

3 files changed

+172
-154
lines changed

‎CCDB/include/CCDB/BasicCCDBManager.h‎

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
#include <unordered_map>
2828
#include <memory>
2929
#include <cstdlib>
30-
#include <vector>
3130

3231
class TGeoManager; // we need to forward-declare those classes which should not be cleaned up
3332

@@ -102,9 +101,9 @@ class CCDBManagerInstance
102101
/// query timestamp
103102
long getTimestamp() const { return mTimestamp; }
104103

105-
/// retrieve an object of type T from CCDB as stored under path and timestamp. Optional to get the headers. Can give a filter of headers to be saved in cache and returned (if present)
104+
/// retrieve an object of type T from CCDB as stored under path and timestamp. Optional to get the headers.
106105
template <typename T>
107-
T* getForTimeStamp(std::string const& path, long timestamp, std::map<std::string, std::string>* headers = nullptr, std::vector<std::string_view> headerFilter = {});
106+
T* getForTimeStamp(std::string const& path, long timestamp, std::map<std::string, std::string>* headers = nullptr);
108107

109108
/// retrieve an object of type T from CCDB as stored under path and using the timestamp in the middle of the run
110109
template <typename T>
@@ -236,7 +235,7 @@ class CCDBManagerInstance
236235
};
237236

238237
template <typename T>
239-
T* CCDBManagerInstance::getForTimeStamp(std::string const& path, long timestamp, std::map<std::string, std::string>* headers, std::vector<std::string_view> headerFilter)
238+
T* CCDBManagerInstance::getForTimeStamp(std::string const& path, long timestamp, std::map<std::string, std::string>* headers)
240239
{
241240
mHeaders.clear(); // we clear at the beginning; to allow to retrieve the header information in a subsequent call
242241
T* ptr = nullptr;
@@ -259,9 +258,7 @@ T* CCDBManagerInstance::getForTimeStamp(std::string const& path, long timestamp,
259258
mFetchedSize += s;
260259
}
261260
}
262-
if (!headerFilter.empty()) {
263-
LOGP(warn, "Header filter ignored when caching is disabled, giving back all headers");
264-
}
261+
265262
if (headers) {
266263
*headers = mHeaders;
267264
}
@@ -278,22 +275,11 @@ T* CCDBManagerInstance::getForTimeStamp(std::string const& path, long timestamp,
278275
ptr = mCCDBAccessor.retrieveFromTFileAny<T>(path, mMetaData, timestamp, &mHeaders, cached.uuid,
279276
mCreatedNotAfter ? std::to_string(mCreatedNotAfter) : "",
280277
mCreatedNotBefore ? std::to_string(mCreatedNotBefore) : "");
281-
282-
// Cache the headers
283-
if (headerFilter.empty()) {
284-
// No filter, cache all headers
285-
for (auto const& h : mHeaders) {
286-
cached.cacheOfHeaders[h.first] = h.second;
287-
}
288-
} else {
289-
// Cache only the asked for headers
290-
for (auto const& k : headerFilter) {
291-
auto it = mHeaders.find(std::string(k));
292-
if (it != mHeaders.end()) {
293-
cached.cacheOfHeaders.insert_or_assign(it->first, it->second); // Only want to overwrite if the header exists in the source
294-
}
295-
}
278+
// update the cached headers
279+
for (auto const& h : mHeaders) {
280+
cached.cacheOfHeaders[h.first] = h.second;
296281
}
282+
// return the cached headers
297283
if (headers) {
298284
*headers = cached.cacheOfHeaders;
299285
}
@@ -331,7 +317,7 @@ T* CCDBManagerInstance::getForTimeStamp(std::string const& path, long timestamp,
331317
size_t s = atol(sh->second.c_str());
332318
mFetchedSize += s;
333319
cached.minSize = std::min(s, cached.minSize);
334-
cached.maxSize = std::max(s, cached.minSize); // I think this should be maxSize, not minSize
320+
cached.maxSize = std::max(s, cached.minSize);
335321
}
336322
} else if (mHeaders.count("Error")) { // in case of errors the pointer is 0 and headers["Error"] should be set
337323
cached.failures++;

‎CCDB/src/CcdbApi.cxx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -880,7 +880,7 @@ bool CcdbApi::retrieveBlob(std::string const& path, std::string const& targetdir
880880

881881
updateMetaInformationInLocalFile(targetpath.c_str(), &headers, &querysummary);
882882
if (outHeaders) {
883-
*outHeaders = std::move(headers); // Re-use the same headers to give back to the callee
883+
*outHeaders = std::move(headers);
884884
}
885885
return true;
886886
}

‎CCDB/test/testCcdbApiHeaders.cxx‎

Lines changed: 162 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -250,147 +250,179 @@ BOOST_AUTO_TEST_CASE(testNonCachedHeaders, *boost::unit_test::precondition(if_re
250250
BOOST_TEST(headers1 != headers2, "The headers for different objects should be different");
251251
}
252252

253-
BOOST_AUTO_TEST_CASE(test_header_filtering, *boost::unit_test::precondition(if_reachable()))
253+
BOOST_AUTO_TEST_CASE(CacheFirstRetrievalAndHeadersPersistence)
254254
{
255-
/// ━━━━━━━ ARRANGE ━━━━━━━━━
256-
// First store objects to test with
257255
test_fixture f;
256+
/// ━━━━━━━ ARRANGE ━━━━━━━━━
257+
// Prepare two validity slots for same path to test ETag change later
258+
std::string path = basePath + "ObjA";
259+
std::string objV1 = "ObjectVersion1";
260+
std::string objV2 = "ObjectVersion2";
261+
std::map<std::string, std::string> meta1{
262+
{"UserKey1", "UValue1"},
263+
{"UserKey2", "UValue2"}};
264+
long v1start = 10'000;
265+
long v1stop = 20'000;
266+
long v2start = v1stop; // contiguous slot
267+
long v2stop = v2start + (v1stop - v1start);
268+
long mid1 = (v1start + v1stop) / 2;
269+
// Store 2 versions
270+
f.api.storeAsTFileAny(&objV1, path, meta1, v1start, v1stop);
271+
f.api.storeAsTFileAny(&objV2, path, meta1, v2start, v2stop);
272+
273+
auto& mgr = o2::ccdb::BasicCCDBManager::instance();
274+
mgr.setURL(ccdbUrl);
275+
mgr.clearCache();
276+
mgr.setCaching(true);
277+
mgr.setFatalWhenNull(true);
278+
mgr.setTimestamp(mid1);
279+
280+
/// ━━━━━━━ACT━━━━━━━━━
281+
std::map<std::string, std::string> headers1, headers2, headers4, headers5;
282+
283+
// 1) First retrieval WITH headers inside 1st slot
284+
auto* p1 = mgr.getForTimeStamp<std::string>(path, mid1, &headers1);
285+
size_t fetchedSizeAfterFirst = mgr.getFetchedSize();
286+
// 2) Second retrieval (cache hit)
287+
auto* p2 = mgr.getForTimeStamp<std::string>(path, mid1, &headers2);
288+
size_t fetchedSizeAfterSecond = mgr.getFetchedSize();
289+
// 3) Third retrieval (cache hit) WITHOUT passing headers
290+
auto* p3 = mgr.getForTimeStamp<std::string>(path, mid1);
291+
// 4) Fourth retrieval with headers again -> should still produce same headers
292+
auto* p4 = mgr.getForTimeStamp<std::string>(path, mid1, &headers4);
293+
// 5) Fifth retrieval with headers again to check persistence
294+
auto* p5 = mgr.getForTimeStamp<std::string>(path, mid1, &headers5);
295+
296+
/// ━━━━━━━ASSERT━━━━━━━━━
297+
298+
BOOST_TEST(p1 != nullptr);
299+
BOOST_TEST(*p1 == objV1);
300+
301+
BOOST_TEST(headers1.count("UserKey1") == 1);
302+
BOOST_TEST(headers1.count("UserKey2") == 1);
303+
BOOST_TEST(headers1["UserKey1"] == "UValue1");
304+
BOOST_TEST(headers1["UserKey2"] == "UValue2");
305+
BOOST_TEST(headers1.count("Valid-From") == 1);
306+
BOOST_TEST(headers1.count("Valid-Until") == 1);
307+
BOOST_TEST(headers1.count("ETag") == 1);
308+
309+
/* Need to manually amend the headers1 to have cache valid until for comparison sake,
310+
* the header is not set in the first request.
311+
* It is only set if the internal cache of CCDB has seen this object before, apperently.
312+
* This will never happen in this test since it was just created and not asked for before.
313+
*/
314+
headers1["Cache-Valid-Until"] = std::to_string(v1stop);
315+
316+
/* In rare cases the header date might be different, if the second has ticked over between the requests
317+
*/
318+
headers1.erase("Date");
319+
headers2.erase("Date");
320+
headers4.erase("Date");
321+
headers5.erase("Date");
258322

259-
std::string pathA = basePath + "CachingA";
260-
std::string pathB = basePath + "CachingB";
261-
std::string ccdbObjO = "testObjectO";
262-
std::string ccdbObjN = "testObjectN";
263-
std::map<std::string, std::string> md = f.metadata;
264-
long start = 1000, stop = 3000;
265-
f.api.storeAsTFileAny<std::string>(&ccdbObjO, pathA, md, start, stop);
266-
f.api.storeAsTFileAny<std::string>(&ccdbObjN, pathB, md, start + 1, stop + 1);
267-
// initilize the BasicCCDBManager
268-
o2::ccdb::BasicCCDBManager& ccdbManager = o2::ccdb::BasicCCDBManager::instance();
269-
ccdbManager.clearCache();
270-
ccdbManager.setURL(ccdbUrl);
271-
ccdbManager.setCaching(true); // This is what we want to test.
272-
273-
// ━━━━━━━━━━━━ ACT ━━━━━━━━━━━━
274-
// Plan: get the objects but also include a string view to indicate which headers to keep
275-
276-
// Convert the stable keys to a vector of string views
277-
std::vector<std::string_view> headerFilter;
278-
for (const auto& k : sStableKeys) {
279-
headerFilter.push_back(k);
280-
}
281-
282-
std::map<std::string, std::string> headers1, headers2, headers3;
283-
auto* obj1 = ccdbManager.getForTimeStamp<std::string>(pathA, (start + stop) / 2, &headers1, headerFilter);
284-
auto* obj2 = ccdbManager.getForTimeStamp<std::string>(pathB, (start + stop) / 2, &headers2, headerFilter);
285-
auto* obj3 = ccdbManager.getForTimeStamp<std::string>(pathA, (start + stop) / 2, &headers3, headerFilter); // Should lead to a cache hit
286-
287-
/// ━━━━━━━━━━━ ASSERT ━━━━━━━━━━━
288-
289-
/// Check that we got something
290-
BOOST_REQUIRE(obj1 != nullptr);
291-
BOOST_REQUIRE(obj2 != nullptr);
292-
BOOST_REQUIRE(obj3 != nullptr);
293-
294-
LOG(debug) << "obj1: " << *obj1;
295-
LOG(debug) << "obj2: " << *obj2;
296-
LOG(debug) << "obj3: " << *obj3;
297-
/// Sanity check
298-
// Check that the objects are correct
299-
BOOST_TEST(*obj1 == ccdbObjO);
300-
BOOST_TEST(*obj3 == ccdbObjO);
301-
BOOST_TEST(obj3 == obj1); // should be the same object in memory since it is cached
302-
BOOST_TEST(obj2 != obj1);
303-
304-
// Modify one and check that the other is also modified
305-
(*obj1) = "ModifiedObject";
306-
BOOST_TEST(*obj1 == "ModifiedObject");
307-
BOOST_TEST(*obj3 == "ModifiedObject");
308-
309-
BOOST_REQUIRE(headers1.size() != 0);
310-
BOOST_REQUIRE(headers3.size() != 0);
323+
BOOST_TEST(p2 == p1); // same pointer for cached scenario
324+
BOOST_TEST(headers2 == headers1); // identical header map
325+
BOOST_TEST(fetchedSizeAfterSecond == fetchedSizeAfterFirst); // no new fetch
311326

312-
LOG(debug) << "Headers1 size: " << headers1.size();
313-
for (const auto& h : headers1) {
314-
LOG(debug) << " " << h.first << " -> " << h.second;
315-
}
316-
LOG(debug) << "Headers3 size: " << headers3.size();
317-
for (const auto& h : headers3) {
318-
LOG(debug) << " " << h.first << " -> " << h.second;
319-
}
327+
BOOST_TEST(p3 == p1);
320328

321-
/// Test the headers keys and values are the same for the two retrievals of the same object
322-
for (const auto& stableKey : sStableKeys) {
323-
LOG(debug) << "Checking header: " << stableKey;
329+
BOOST_TEST(p4 == p1);
330+
BOOST_TEST(headers4 == headers1);
324331

325-
BOOST_TEST(headers1.count(stableKey) > 0, "stable key missing in headers1: " + stableKey);
326-
BOOST_TEST(headers3.count(stableKey) > 0, "stable key missing in headers3: " + stableKey);
332+
// Mutate the returned header map locally and ensure it does not corrupt internal cache
333+
headers4["UserKey1"] = "Tampered";
334+
BOOST_TEST(p5 == p1);
335+
BOOST_TEST(headers5["UserKey1"] == "UValue1"); // internal unchanged
336+
}
327337

328-
BOOST_TEST(headers1.at(stableKey) == headers3.at(stableKey));
329-
}
338+
BOOST_AUTO_TEST_CASE(FailedFetchDoesNotGiveMetadata)
339+
{
340+
test_fixture f;
330341

331-
// Now test that the filtering worked, i.e. that we only have the stable keys
332-
for (const auto& h : headers1) {
333-
BOOST_TEST(sStableKeys.count(h.first) > 0, "Found non-stable key in headers1: " + h.first);
334-
}
335-
for (const auto& h : headers2) {
336-
BOOST_TEST(sStableKeys.count(h.first) > 0, "Found non-stable key in headers2: " + h.first);
337-
}
338-
for (const auto& h : headers3) {
339-
BOOST_TEST(sStableKeys.count(h.first) > 0, "Found non-stable key in headers3: " + h.first);
340-
}
342+
/// ━━━━━━━ ARRANGE ━━━━━━━━━
343+
std::string path = basePath + "FailThenRecover";
344+
std::string content = "ContentX";
345+
std::map<std::string, std::string> meta{{"Alpha", "Beta"}};
346+
long s = 300'000, e = 310'000;
347+
f.api.storeAsTFileAny(&content, path, meta, s, e);
348+
auto& mgr = o2::ccdb::BasicCCDBManager::instance();
349+
mgr.clearCache();
350+
mgr.setCaching(true);
351+
mgr.setFatalWhenNull(false);
352+
353+
/// ━━━━━━━ ACT ━━━━━━━━━
354+
// Intentionally pick a timestamp outside validity to fail first
355+
long badTS = s - 1000;
356+
long goodTS = (s + e) / 2;
357+
std::map<std::string, std::string> hFail, hGood;
358+
auto* badObj = mgr.getForTimeStamp<std::string>(path, badTS, &hFail);
359+
auto* goodObj = mgr.getForTimeStamp<std::string>(path, goodTS, &hGood);
360+
361+
/// ━━━━━━━ ASSERT ━━━━━━━━━
362+
BOOST_TEST(!hFail.empty()); // Should have some headers
363+
BOOST_TEST(hFail["Alpha"] != "Beta"); // But not the metadata
364+
BOOST_TEST(hGood.count("Alpha") == 1);
365+
BOOST_TEST(hGood["Alpha"] == "Beta");
366+
367+
mgr.setFatalWhenNull(true);
368+
}
341369

342-
// Now check that we just have the filtered keys
343-
BOOST_TEST(headers1.size() == sStableKeys.size());
344-
BOOST_TEST(headers3.size() == sStableKeys.size());
370+
BOOST_AUTO_TEST_CASE(FirstCallWithoutHeadersThenWithHeaders)
371+
{
372+
test_fixture f;
345373

346-
// Extract the keys and values so that we can compare the sets
347-
std::set<std::string> header1Keys, header2Keys, header3Keys;
348-
std::set<std::string> header1Values, header2Values;
374+
std::string path = basePath + "LateHeaders";
375+
std::string body = "Late";
376+
std::map<std::string, std::string> meta{{"LateKey", "LateVal"}};
377+
long s = 400'000, e = 410'000;
378+
f.api.storeAsTFileAny(&body, path, meta, s, e);
379+
380+
auto& mgr = o2::ccdb::BasicCCDBManager::instance();
381+
mgr.clearCache();
382+
mgr.setCaching(true);
383+
long ts = (s + e) / 2;
384+
385+
// 1) First call with nullptr headers
386+
auto* first = mgr.getForTimeStamp<std::string>(path, ts);
387+
BOOST_TEST(first != nullptr);
388+
BOOST_TEST(*first == body);
389+
390+
// 2) Second call asking for headers - should return the full set
391+
std::map<std::string, std::string> h2;
392+
auto* second = mgr.getForTimeStamp<std::string>(path, ts, &h2);
393+
BOOST_TEST(second == first);
394+
BOOST_TEST(h2.count("LateKey") == 1);
395+
BOOST_TEST(h2["LateKey"] == "LateVal");
396+
BOOST_TEST(h2.count("Valid-From") == 1);
397+
BOOST_TEST(h2.count("Valid-Until") == 1);
398+
}
349399

350-
for (const auto& h : headers1) {
351-
header1Keys.insert(h.first);
352-
}
353-
for (const auto& h : headers2) {
354-
header2Keys.insert(h.first);
355-
}
356-
for (const auto& h : headers3) {
357-
header3Keys.insert(h.first);
358-
}
400+
BOOST_AUTO_TEST_CASE(HeadersAreStableAcrossMultipleHits)
401+
{
402+
test_fixture f;
359403

360-
BOOST_TEST(header1Keys.size() == sStableKeys.size());
361-
BOOST_TEST(header2Keys.size() == sStableKeys.size());
362-
BOOST_TEST(header3Keys.size() == sStableKeys.size());
363-
364-
BOOST_TEST(header1Keys == header2Keys, boost::test_tools::per_element());
365-
BOOST_TEST(header2Keys == header3Keys, boost::test_tools::per_element());
366-
BOOST_TEST(header1Keys == header3Keys, boost::test_tools::per_element());
367-
368-
BOOST_TEST(header1Keys == sStableKeys, boost::test_tools::per_element());
369-
BOOST_TEST(header2Keys == sStableKeys, boost::test_tools::per_element());
370-
BOOST_TEST(header3Keys == sStableKeys, boost::test_tools::per_element());
371-
372-
/// Make sure that headers for different objects are different
373-
BOOST_TEST(headers1 != headers2, "The headers for different objects should be different (mainly ETag)");
374-
375-
// Make a set of keys that we know beforehand is the same for different objects
376-
std::set<std::string> keysThatShouldBeDifferent = {
377-
"Valid-From",
378-
"Valid-Until",
379-
"Created",
380-
"ETag",
381-
"Content-Disposition",
382-
"Content-Location",
383-
"path",
384-
"Content-MD5",
385-
};
386-
387-
for (const auto& stableKey : keysThatShouldBeDifferent) {
388-
BOOST_TEST(headers2.count(stableKey) > 0, "stable key missing in headers2: " + stableKey);
389-
header2Values.insert(headers2.at(stableKey));
390-
header1Values.insert(headers1.at(stableKey));
404+
std::string path = basePath + "StableHeaders";
405+
std::string body = "Stable";
406+
std::map<std::string, std::string> meta{{"HK", "HV"}};
407+
long s = 500'000, e = 510'000;
408+
f.api.storeAsTFileAny(&body, path, meta, s, e);
409+
410+
auto& mgr = o2::ccdb::BasicCCDBManager::instance();
411+
mgr.clearCache();
412+
mgr.setCaching(true);
413+
long ts = (s + e) / 2;
414+
415+
std::map<std::string, std::string> h1;
416+
auto* o1 = mgr.getForTimeStamp<std::string>(path, ts, &h1);
417+
BOOST_TEST(o1 != nullptr);
418+
BOOST_TEST(h1.count("HK") == 1);
419+
420+
std::string etag = h1["ETag"];
421+
for (int i = 0; i < 15; ++i) {
422+
std::map<std::string, std::string> hi;
423+
auto* oi = mgr.getForTimeStamp<std::string>(path, ts, &hi);
424+
BOOST_TEST(oi == o1);
425+
BOOST_TEST(hi.count("HK") == 1);
426+
BOOST_TEST(hi["ETag"] == etag);
391427
}
392-
393-
BOOST_TEST(header2Values.size() == keysThatShouldBeDifferent.size());
394-
395-
BOOST_TEST(header2Values != header1Values, boost::test_tools::per_element());
396428
}

0 commit comments

Comments
 (0)