-
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
Add try/catch in OnlineBeamMonitor plugin to resolve runtime error #40716
Add try/catch in OnlineBeamMonitor plugin to resolve runtime error #40716
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40716/34101
|
A new Pull Request was created by @francescobrivio for master. It involves the following packages:
@malbouis, @pmandrik, @emanueleusai, @tvami, @cmsbuild, @saumyaphor4252, @syuvivida, @ggovi, @francescobrivio, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
aff7d49
to
3bb9db9
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40716/34105
|
Pull request #40716 was updated. @malbouis, @pmandrik, @emanueleusai, @tvami, @cmsbuild, @saumyaphor4252, @syuvivida, @ggovi, @francescobrivio, @micsucmed, @rvenditti can you please check and sign again. |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-80b02d/30471/summary.html Comparison SummarySummary:
|
agree 12_6 backport not needed if collisions will use 13_X |
+1 |
@cmsbuild, please abort |
@cms-sw/db-l2 unit tests have finally passed :) log. |
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-80b02d/30539/summary.html Comparison SummarySummary:
|
+1
|
The CMS coding rules state explicitly
|
resolves #39948
PR description:
This PR addesses the issue observed in a technical test by @missirol and reported in #39948 where the
BeamOnlineMonitor
is run offline (although it's meant to be run online only) on data older than Run 3.The solution implemented was suggested by @mmusich in #39948 (comment) and it's basically copied (with minor modifications) from the same try/catch approach used in the
BeamSpotOnline_PayloadInspector
plugin.The
[RFC]
is beacuse I'm not sure if adding the functions for this check directly in the.cc
is fine or not.The solution nonetheless works as expected.
PR validation:
Successfully tested using the recipe prodvided by Marino in #39948 (comment): the
Fatal Exception
is now gone and replaced by aLogError
message just like in the Payload Inspector plugin.Backport:
Not a backport.
I'm reluctant to provide a backport to 12_6_X beacuse online the BeamSpot is computed with collision only, and data taking for
pp
data will happen in 13_0_X AFAIU. Of course if @cms-sw/dqm-l2 (or others) disagree I'm happy to provide a 12_6_X backport as well.FYI @dzuolo @gennai @lguzzi