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

Hcal DQM Migration - II.2 #6337

Merged
merged 7 commits into from Nov 17, 2014
Merged

Conversation

cowden
Copy link

@cowden cowden commented Nov 11, 2014

This should complete the HCAL DQM migration to MT framework. This affects DQM/HcalMonitorClient and DQM/HcalMonitorTasks. The use of helper classes inheriting from a common base class made it dificult to trace for every case the functional flow of the monitors and clients. However, with the exception of the removal of endRun(void) methods in HcalBaseDQClient and its derivatives, the functionality is intended to be the same in both frameworks.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @cowden (Christopher Cowden) for CMSSW_7_3_X.

Hcal DQM Migration - II.2

It involves the following packages:

DQM/HcalMonitorClient
DQM/HcalMonitorTasks

@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 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

@deguio
Copy link
Contributor

deguio commented Nov 12, 2014

hello @cowden
thanks. I see that not all my comments have been addressed:

  • please remove HcalBaseDQMonitor::endJob. it won’t be triggered anyway.
  • please move what you do in HcalBaseDQMonitor::beginJob in the actor.
  • in many cases (~all the helper classes) I see that not all the pointers allocated with new are deleted in the destructor. please make sure that the code doesn't leak by adding the delete or by using smart pointers
  • please adjust the indentation: sometimes the code is not easily readable. see for example: HcalSummaryClient
    cheers,
    F.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@deguio
Copy link
Contributor

deguio commented Nov 17, 2014

+1
it seems to me that all my comments have been implemented.

@cmsbuild
Copy link
Contributor

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). This pull request will be automatically merged.

cmsbuild added a commit that referenced this pull request Nov 17, 2014
@cmsbuild cmsbuild merged commit d30b791 into cms-sw:CMSSW_7_3_X Nov 17, 2014
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

3 participants