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

Default format for new DB in output reverted to ORA #2172

Merged
merged 4 commits into from Feb 2, 2014

Conversation

ggovi
Copy link
Contributor

@ggovi ggovi commented Jan 25, 2014

The auto-generation of Condition DB schemas is reverted to construct by default the old layout (ORA). In order to generate output with the new format, the environment variable NEW_CONDDB has to be set.
Additional fixes concern:

  • test for fronTier
  • MetadataService error policy for non-existing tags

Packages involved:
CondCore/CondDB
CondCore/IOVService
CondCore/MetaDataService
CondCore/Utilities

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ggovi for CMSSW_7_0_X.

Default format for new DB in output reverted to ORA

It involves the following packages:

CondCore/CondDB
CondCore/IOVService
CondCore/MetaDataService
CondCore/Utilities

@apfeiffer1, @diguida, @cmsbuild, @nclopezo, @rcastello, @ggovi, @Degano can you please review it and eventually sign? Thanks.
@ghellwig this is something you requested to watch as well.
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.
@ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@diguida
Copy link
Contributor

diguida commented Jan 25, 2014

This was discussed yesterday. The CSA14 calibrations will be produced in 70X, but the new format will be deployed only after the DIGI-RECO campaign will be over. As a consequence, the calibrations should be produced in 70X by default in the old format. 71X is supposed to have the new system as default, since it will be used for validating it. Hence, please do not propagate this PR in 71X: it is intended only for 70X.

@ggovi
Copy link
Contributor Author

ggovi commented Jan 25, 2014

Salvatore, thanks for the clarification. However, i was wondering if it was actually better to keep the ORA output in 7_1 as well UNTIL we will have deployed the new DropBox for the new DB. For specific tests, we can always set the environment variable on. In addition, this PR also contains fixes ( although small ones ) outside the DB format.

std::unique_ptr<IGTSchema> gtSchema( new GTSchema( coralSession->nominalSchema() ) );
bool newCondDb = iovSchema->exists();
if( !newCondDb && !readOnly ){
const char* new_conddb_env = ::getenv( NEW_CONDDB );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getenv is taboo. Please use the parameter set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Chris,

[...]

  • bool newCondDb = iovSchema->exists();
    
  • if( !newCondDb && !readOnly ){
    
  •   const char\* new_conddb_env = ::getenv( NEW_CONDDB );
    

getenv is taboo. Please use the parameter set.

this variable will be a temporary fix and is used only by experts for
testing and validating.

And as it is mainly used in standalone executables (condb_...) could you
please explain how to use the parameter set there in an easy way ?

Thanks,
cheers, andreas

@cmsbuild
Copy link
Contributor

@apfeiffer1
Copy link
Contributor

+1

@ggovi
Copy link
Contributor Author

ggovi commented Jan 27, 2014

Hi,

to provide a fast solution I have temporarily removed the ENV-based option

@cmsbuild
Copy link
Contributor

Pull request #2172 was updated. @apfeiffer1, @diguida, @cmsbuild, @nclopezo, @rcastello, @ggovi, @Degano can you please check and sign again.

@ktf
Copy link
Contributor

ktf commented Jan 27, 2014

This looks ok to me. Please remember to bring it up in ORP tomorrow.

@cmsbuild
Copy link
Contributor

@apfeiffer1
Copy link
Contributor

+1

@diguida
Copy link
Contributor

diguida commented Jan 27, 2014

Hi @ggovi !
We discussed this in the meeting today. We agreed ( @apfeiffer1 can further comment, I could have misunderstood) to keep for the moment also 7.1.X using the OLD system by default (developers can switch it on for their tests). Then, when the final steps of the internal verification/validation are done, I think it would be good to switch the default in 7.1.X to the new system: in this way, you can start doing the complete integration and validation. At that point (end of Q1) also the dropbox for the new system should be in place, so it is safe to switch: developers creating calibrations in 7.1.X will be warned to use the new dropbox.
Does this seem reasonable to you all?

@apfeiffer1
Copy link
Contributor

+1

On Mon, Jan 27, 2014 at 9:23 PM, Salvatore Di Guida <
notifications@github.com> wrote:

Hi @ggovi https://github.com/ggovi !
We discussed this in the meeting today. We agreed ( @apfeiffer1https://github.com/apfeiffer1can further comment, I could have misunderstood) to keep for the moment
also 7.1.X using the OLD system by default (developers can switch it on for
their tests). Then, when the final steps of the internal
verification/validation are done, I think it would be good to switch the
default in 7.1.X to the new system: in this way, you can start doing the
complete integration and validation. At that point (end of Q1) also the
dropbox for the new system should be in place, so it is safe to switch:
developers creating calibrations in 7.1.X will be warned to use the new
dropbox.
Does this seem reasonable to you all?


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

Thanks,
cheers, andreas

@cmsbuild
Copy link
Contributor

Pull request #2172 was updated. @apfeiffer1, @diguida, @cmsbuild, @nclopezo, @rcastello, @ggovi, @Degano can you please check and sign again.

@apfeiffer1
Copy link
Contributor

+1
apparently my previous "+1" on the latest commit does not count (though it is equivalent to this one, mathematically).

@cmsbuild
Copy link
Contributor

@diguida
Copy link
Contributor

diguida commented Jan 31, 2014

+1
@ggovi and I run the test.
TODO: understand why our tests did not spot the issue, even if the setup is the same.

@cmsbuild
Copy link
Contributor

Pull request #2172 was updated. @ggovi, @cmsbuild, @apfeiffer1, @Degano, @nclopezo can you please check and sign again.

@apfeiffer1
Copy link
Contributor

+1
provided Jenkins agrees ...

On Fri, Jan 31, 2014 at 3:47 PM, cmsbuild notifications@github.com wrote:

Pull request #2172 #2172 was
updated. @ggovi https://github.com/ggovi, @cmsbuildhttps://github.com/cmsbuild,
@apfeiffer1 https://github.com/apfeiffer1, @deganohttps://github.com/degano,
@nclopezo https://github.com/nclopezo can you please check and sign
again.

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

Thanks,
cheers, andreas

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

+1

ktf added a commit that referenced this pull request Feb 2, 2014
Db -- Default format for new DB in output reverted to ORA
@ktf ktf merged commit 0b670a8 into cms-sw:CMSSW_7_0_X Feb 2, 2014
@nclopezo nclopezo added this to the CMSSW_7_0_0 milestone Feb 7, 2014
@nclopezo nclopezo removed this from the CMSSW_7_0_0_pre13 milestone Feb 7, 2014
ggovi pushed a commit to ggovi/cmssw that referenced this pull request Jan 11, 2017
Advance data for RecoJets/JetProducers.
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

7 participants