-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
New template one::EDAnalyzer for dealing with B ON/OFF transitions using EventSetup and PoolDBOutputService. #16248
New template one::EDAnalyzer for dealing with B ON/OFF transitions using EventSetup and PoolDBOutputService. #16248
Conversation
A new Pull Request was created by @diguida (Salvatore Di Guida) for CMSSW_8_1_X. It involves the following packages: CondCore/DBOutputService @ghellwig, @cerminar, @cmsbuild, @franzoni, @ggovi, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
please test |
The tests are being triggered in jenkins. |
@smuzaffar question: what does
mean in #16248 (comment) |
seems it has been introduced here: |
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
|
…ntSetup and PoolDBOutputService. An example is provided for EcalADCToGeVConstant.
…ols/RunInfo: it is a tool manipulating tags based on a condition on the magnet current as seen in the RunInfo payloads.
f0cc358
to
3e05de0
Compare
Pull request #16248 was updated. @ggovi, @cmsbuild, @davidlange6 can you please check and sign again. |
Small fixes:
|
please test |
The tests are being triggered in jenkins. |
+1 |
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
|
@diguida, please ignore that. I was testing cms-bot reporting script. |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar |
+1 |
public: | ||
BTransitionAnalyzer( const edm::ParameterSet& pset ): | ||
m_currentThreshold( pset.getUntrackedParameter<double>( "currentThreshold", 18000. ) ) { | ||
usesResource( "PoolDBOutputService" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diguida I have just one comment/question. You declare that you use the resource "PoolDBOutputService". As I understood, this is only needed, if the resource is not thread-safe. However, @Dr15Jones commented in this HN thread that it appears to be thread-safe.
Do you have reasons to declare it anyways, except that "appears to be thread-safe" might be a bit vague?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghellwig sorry for late reply.
As discussed few minutes ago at the AlCaDB meeting, the code here was originally developed in 7.4.X, where PoolDBOutputService
was not thread safe.
According to the API description
The system makes sure that only one module which has declared a usage of a specific resource will ever be run at a time. That is, the system will serialize all calls to all modules which declare that resource.
In the online context of the O2O, a cmsRun
job will call only this module (apart from input sources), so, even if formally the performance penalty is introduced, operationally it should not be visible. Besides this, in that context you care more of the correctness of the database transactions than of the performance of job: if you have a fast job potentially writing wrong IOV, this is a big problem.
Anyhow, if also @Dr15Jones agrees that the usesResource
is not needed, I can put a PR removing it.
HTH
P.S.: you were also saying another observation at the meeting, but I did not catch it. Feel free to post it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diguida thanks for the clarification!
My other comment does not apply anymore, but I list it here for reference: You write usesResource("PoolDBOutputService");
, but in general it is better practice to define a static const string in PoolDBOutputService and use this to avoid typos because the compiler checks the name of the variable which is not the case if you use a hard-coded string everytime the resource is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghellwig you are absolutely right! This is common sense (as well as a software best practice):
- adding a static data member using constant expression for labelling the framework service name:
static constexpr auto serviceName = "PoolDBOutputService";
- using it every time you refer to the service name in client code:
usesResource( std::string( PoolDBOutputService::serviceName ) );
I used 'appears' just to give me wiggle room in case I missed something. The code is definitely intended to be thread safe so I would therefore suggest not using a 'SharedResource' for the Service. If we later discover a thread-safety problem, we'll treat that as a bug in the Service and fix the Service. |
@Dr15Jones thanks for looking into this. I also owe you a reply in a private thread on a similar topic: sorry about that, will come back to you later today. |
FYI: #16427 |
This code enables to automatically handle condition changes in case of magnetic field transitions using
EventSetup
via GlobalTag, andPoolDBOutputService
. It was developed during 2015.The new analyzer requires as template arguments:
It grabs the whole information using the GlobalTag at
endRun
:RunInfoRcd
PoolDBOutputService
.A complete example is provided for the
EcalADCToGeVConstant
class.@mmusich @franzoni this is something you might want to watch as well.