Skip to content

CcdbApi: Fixed logReading in vectoredLoadFileTomemory#12276

Merged
shahor02 merged 4 commits into
AliceO2Group:devfrom
TrifleMichael:CcdbApiImprovedLogging
Nov 27, 2023
Merged

CcdbApi: Fixed logReading in vectoredLoadFileTomemory#12276
shahor02 merged 4 commits into
AliceO2Group:devfrom
TrifleMichael:CcdbApiImprovedLogging

Conversation

@TrifleMichael
Copy link
Copy Markdown
Contributor

In order do properly print data received from headers in logReading, the line calling logReading has been moved after the execution of downloader loop.

Additionally the warning of "No content received" has been demoted to info, as it is expected in various use cases. For example in case of checking whether data has been modified and receiving 304.

Comment thread CCDB/src/CcdbApi.cxx
// Save snapshots
for (int i = 0; i < requestContexts.size(); i++) {
auto& requestContext = requestContexts.at(i);
logReading(requestContext.path, requestContext.timestamp, &requestContext.headers,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@TrifleMichael will it show the actual object fetched (with timestamps and etag) like it was before vectorization?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will show the full path including the timestamp and etag like it did before. The problem was calling logReading before the headers arrived, which resulted in requestContext.headers being empty and not containing that information.

Comment thread CCDB/src/CcdbApi.cxx Outdated
}
} else {
LOG(warning) << "Did not receive content for " << requestContext.path << "\n"; // Temporarily demoted to warning, since it floods the infologger
LOG(info) << "Did not receive content for " << requestContext.path << "\n"; // This is to be expected in case of http answer 304 and similar cases
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Cannot we suppress the message completely when the content is not supposed to be shipped?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can definitely do that in case of receiving http 304. I don't know if there are any other similar cases, but most likely that could be done.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Only 304 is a legitimate case of not shipping an object, and we don't need to log this case. All other cases I know should produce a LOG(error).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Then I'll modify the code so it log's an error in case of non-304 response and no object received.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In principle only http codes >= 400 should be considered errors and returned. Or libcurl IO errors, if the call could not be performed at all.

@shahor02
Copy link
Copy Markdown
Collaborator

@costing, don't all cases other than 304 already produce an error message:

} else if (response_code == 404) {
LOG(error) << "Requested resource does not exist: " << url;
errorflag = true;
} else {
LOG(error) << "Error in fetching object " << url << ", curl response code:" << response_code;
errorflag = true;
}
?

@costing
Copy link
Copy Markdown
Collaborator

costing commented Nov 20, 2023

@shahor02 the above if clauses on various ranges of response codes should indeed filter those out ([200, 300) = ok, [300, 400) = redirects, 404 = not found) leaving essentially only other 40x/50x codes to log as an error on the last else clause. And we should be verbose about those as they should not be encountered and we need as much information as possible to understand the context when they might occur.

@shahor02
Copy link
Copy Markdown
Collaborator

@costing So, then it should be ok to log an error in the https://github.com/AliceO2Group/AliceO2/pull/12276/files/6ba1cfa64111499e28a87f1b34eff43647708eaa#diff-2355211fcd8f9232fbeddc19348089f206c655459237baffc0252dbd544dd025R1691 only if a curl code assuming a shipped object was received but the object was not there, right? Effectively response in [200 : 300[ with no payload, since all other cases are already covered by the navigateURLsAndRetrieveContent logs).

@alibuild
Copy link
Copy Markdown
Collaborator

Error while checking build/O2/fullCI for 6ba1cfa at 2023-11-21 22:05:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'


## sw/BUILD/O2Physics-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'


## sw/BUILD/o2checkcode-latest/log
--
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
/sw/SOURCES/O2/12276-slc8_x86-64/0/Generators/src/GeneratorPythia8.cxx:459:32: error: statement should be inside braces [readability-braces-around-statements]
/sw/SOURCES/O2/12276-slc8_x86-64/0/Generators/src/GeneratorPythia8.cxx:461:32: error: statement should be inside braces [readability-braces-around-statements]
++ [[ 0 == 0 ]]
++ exit 1
--

Full log here.

@TrifleMichael
Copy link
Copy Markdown
Contributor Author

@shahor02 I'll update this PR so that errors are logged only if 400+ was received and no content arrived.

shahor02
shahor02 previously approved these changes Nov 23, 2023
Copy link
Copy Markdown
Collaborator

@shahor02 shahor02 left a comment

Choose a reason for hiding this comment

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

Thanks!

@shahor02 shahor02 merged commit 9068537 into AliceO2Group:dev Nov 27, 2023
mwinn2 pushed a commit to mwinn2/AliceO2 that referenced this pull request Apr 25, 2024
…2276)

* CcdbApi: Fixed logReading in vectoredLoadFileTomemory

* CcdbDownloader: Better transfer info logging

* Formatting fix

* Removed whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants