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

FastTimerService: remove obsolete parameters #5100

Merged

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Aug 29, 2014

Also, add some more clock_gettime-based timers under test

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fwyzard (Andrea Bocci) for CMSSW_7_2_X.

FastTimerService: remove obsolete parameters

It involves the following packages:

HLTrigger/Timer

@Martin-Grunewald, @perrotta, @cmsbuild, @nclopezo, @fwyzard 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.

@cmsbuild
Copy link
Contributor

@fwyzard
Copy link
Contributor Author

fwyzard commented Aug 29, 2014

+1

On 29 August 2014 11:42, cmsbuild notifications@github.com wrote:

+1
Tested at: 3e3007a
3e3007a

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5100/729/summary.html


Reply to this email directly or view it on GitHub
#5100 (comment).

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

@cmsbuild
Copy link
Contributor

@Martin-Grunewald
Copy link
Contributor

-1

This breaks our tests! So it needs a PR in 71X as well and ConfDB parsing!

@fwyzard
Copy link
Contributor Author

fwyzard commented Aug 29, 2014

Hi Martin,
I see.

Yes, I guess you do need to first make the change in ConfDB, but there
should not be any need for a 7.1.x PR: the parameters are never read and
have a default value, so the modules should not complain if the go away.

.A

On 29 August 2014 12:53, Martin Grunewald notifications@github.com wrote:

-1

This breaks our tests! So it needs a PR in 71X as well and ConfDB parsing!


Reply to this email directly or view it on GitHub
#5100 (comment).

@Martin-Grunewald
Copy link
Contributor

Well, to reduce work, we use 71X parsings for both 71X and 72X (otherwise
we need to migrate menus twice for HLT developments). That's why
it should be in 71X also, so that the parsing picks up on it (without, it crashes
due to extra parameters when the PR is in in 72X).
I'd also like to delay parsing until this JSON nightmare is sorted, thus can we
delay this PR or is it urgent? If it is, can we split it and at least delay the parameter
removal?
Thanks!

@fwyzard
Copy link
Contributor Author

fwyzard commented Aug 29, 2014

Hi Martin,
it's not urgent, as long as in enters 7.2.x .

Ciao,
.Andrea
On 29 Aug 2014 16:12, "Martin Grunewald" notifications@github.com wrote:

Well, to reduce work, we use 71X parsings for both 71X and 72X (otherwise
we need to migrate menus twice for HLT developments). That's why
it should be in 71X also, so that the parsing picks up on it (without, it
crashes
due to extra parameters when the PR is in in 72X).
I'd also like to delay parsing until this JSON nightmare is sorted, thus
can we
delay this PR or is it urgent? If it is, can we split it and at least
delay the parameter
removal?
Thanks!


Reply to this email directly or view it on GitHub
#5100 (comment).

@Martin-Grunewald
Copy link
Contributor

OK, Just to know: can I make an identical PR for 71X or
is there anything different code wise or preventing this
to enter 71X?

@Martin-Grunewald
Copy link
Contributor

If so I just make a PR with ONLY the parameter removal for 71X

@fwyzard
Copy link
Contributor Author

fwyzard commented Aug 29, 2014

I'll make a PR.

Ciao,
.Andrea
On 29 Aug 2014 16:27, "Martin Grunewald" notifications@github.com wrote:

If so I just make a PR with ONLY the parameter removal for 71X


Reply to this email directly or view it on GitHub
#5100 (comment).

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

@Martin-Grunewald
Copy link
Contributor

@ktf - this is now fully signed, please integrate....

ktf added a commit that referenced this pull request Sep 1, 2014
…e_parameters

FastTimerService: remove obsolete parameters
@ktf ktf merged commit aa9b708 into cms-sw:CMSSW_7_2_X Sep 1, 2014
@fwyzard fwyzard deleted the FastTimerService_remove_obsolete_parameters branch December 23, 2014 10:27
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

4 participants