Skip to content
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

RPC DQM migration to DQMEDHarvester #5410

Merged
merged 2 commits into from Oct 8, 2014

Conversation

acimmino
Copy link
Contributor

RPC clients have been migrated to the DQMEDHarvester interface as described in:
https://twiki.cern.ch/twiki/bin/viewauth/CMS/ThreadedDQM

Also removed obsolete modules that were not used anymore

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @acimmino (Anna Cimmino) for CMSSW_7_3_X.

RPC DQM migration to DQMEDHarvester

It involves the following packages:

DQM/RPCMonitorClient

@nclopezo, @danduggan, @rovere, @cmsbuild, @deguio, @ojeda can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@deguio
Copy link
Contributor

deguio commented Sep 29, 2014

ciao anna,
many thanks for this. just a couple of minor comments:

  • it seems to me that the class RPCQualityTests is not needed. am I wrong? is it possible to remove it?
  • in RPCRecHitProbabilityClient could you remove the instances of dbe_?

apart from this I think that the PR is ready to be merged.
thanks,
F.

@acimmino
Copy link
Contributor Author

Ciao Federico,

I'm making the changes you proposed. Can I profit of this occasion and make a very small bug fix to DQM/RPCMonitorDigi/src/RPCMonitorDigi.C. The bug fix I'd like to make is

OLD Lines:
474: int xbin = wheelOrDiskNumber+3;
475: if (region==-1) xbin = wheelOrDiskNumber+4;
476: if(meRegion[os.str()]) meRegion[os.str()]->Fill(xbin,ring,numDigi);

NEW Lines:
474: int xbin = wheelOrDiskNumber+ numberOfDisks_ ;
475: if (region==-1) {xbin = wheelOrDiskNumber+ numberOfDisks_ +1;}
476: if(meRegion[os.str()]) {meRegion[os.str()]->Fill(xbin,ring,numDigi);}

If you prefer, I can put it in a subsequent PR instead.
Cheers,
Anna


From: deguio [notifications@github.com]
Sent: 29 September 2014 17:32
To: cms-sw/cmssw
Cc: Anna Cimmino
Subject: Re: [cmssw] RPC DQM migration to DQMEDHarvester (#5410)

ciao anna,
many thanks for this. just a couple of minor comments:

  • it seems to me that the class RPCQualityTests is not needed. am I wrong? is it possible to remove it?
  • in RPCRecHitProbabilityClient could you remove the instances of dbe_?

apart from this I think that the PR is ready to be merged.
thanks,
F.


Reply to this email directly or view it on GitHubhttps://github.com//pull/5410#issuecomment-57179027.

@deguio
Copy link
Contributor

deguio commented Sep 30, 2014

it is fine. we can merge all together.
thanks,
F.

@deguio
Copy link
Contributor

deguio commented Oct 2, 2014

not sure if I have to wait for additional commits here. let me know if this is not the case.
thanks,
F.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes or unless it breaks tests. @nclopezo, @ktf can you please take care of it?

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 8, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 8, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

nclopezo added a commit that referenced this pull request Oct 8, 2014
DQM/RPCMonitorClient -- RPC DQM migration to DQMEDHarvester
@nclopezo nclopezo merged commit 5ef88f3 into cms-sw:CMSSW_7_3_X Oct 8, 2014
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 8, 2014

@ktf
Copy link
Contributor

ktf commented Oct 9, 2014

I think this broke the IBs.

----- Begin Fatal Exception 09-Oct-2014 00:08:39 CEST-----------------------
An exception of category 'PluginNotFound' occurred while
   [0] Constructing the EventProcessor
Exception Message:
Unable to find plugin 'RPCFEDIntegrity' in category 'CMS EDM Framework Module'. Please check spelling of name.
----- End Fatal Exception -------------------------------------------------

can you please check?

@deguio
Copy link
Contributor

deguio commented Oct 9, 2014

ciao @ktf @acimmino
I have realized the same few minutes ago. the reason is the removal of the lines at:
https://github.com/cms-sw/cmssw/pull/5410/files#diff-f81dbe13b162bb67239a55a1d20aff61L10

anna, could you provide a fix for this? was the removal of DQM/RPCMonitorClient/src/SealModule.cc accidental?

to reproduce the problem you can runTheMatrix.py -l 4.78

thanks,
F.

@ktf
Copy link
Contributor

ktf commented Oct 10, 2014

Any news on this? I'll revert everything otherwise.

@davidlange6
Copy link
Contributor

seems we should revert if no fix by 1400 IB then we have a good basis for the weekend..

On Oct 10, 2014, at 10:55 AM, Giulio Eulisse notifications@github.com wrote:

Any news on this? I'll revert everything otherwise.


Reply to this email directly or view it on GitHub.

@acimmino
Copy link
Contributor Author

Sorry,

I was internetless from Friday onwards. I'll provide a fix by Monday. Thanks for reverting in the meantime.

Cheers,
Anna


From: David Lange [notifications@github.com]
Sent: 10 October 2014 12:13
To: cms-sw/cmssw
Cc: Anna Cimmino
Subject: Re: [cmssw] RPC DQM migration to DQMEDHarvester (#5410)

seems we should revert if no fix by 1400 IB then we have a good basis for the weekend..

On Oct 10, 2014, at 10:55 AM, Giulio Eulisse notifications@github.com wrote:

Any news on this? I'll revert everything otherwise.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/5410#issuecomment-58637001.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants