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

fix #970, #12317: adjust dbpatch table; delete map values with triggers #2893

Merged
merged 14 commits into from Aug 5, 2014

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Jul 31, 2014

Fixes http://trac.openmicroscopy.org.uk/ome/ticket/970 and http://trac.openmicroscopy.org.uk/ome/ticket/12317. Broken tests not of this PR are fixed by @ximenesuk in #2892 and ome/omero-scripts#91.
--no-rebase

To test:

  1. create a fresh DB
  2. observe the contents of the configuration and dbpatch tables
  3. start the server
  4. see that a DBEnumCheck entry is added to both tables
  5. check that the server is running okay
  6. restart the server
  7. see that both tables have nothing new from the restart
  8. check that the server is running okay
  9. import some images
  10. see that we thus have some map property entries in the database, e.g., in the uploadjob_versioninfo table
  11. delete some images
  12. see that the corresponding property map entries are deleted.

@mtbc
Copy link
Member Author

mtbc commented Jul 31, 2014

Could I have a breaking label added, please?

@mtbc
Copy link
Member Author

mtbc commented Jul 31, 2014

Are the --depends-on being ignored by OME-5.1-breaking-push?

@sbesson
Copy link
Member

sbesson commented Jul 31, 2014

No, apart from being informative, --depends-on is only parsed by scc travis-merge to include multiple PRs in the breaking queue, they all need to be labelled as breaking.

@mtbc
Copy link
Member Author

mtbc commented Jul 31, 2014

Ah, thank you, good to know.

Well, this PR seems to be consistently all green apart from the tests that @ximenesuk kindly fixed after #2847 broke them: want to remove the breaking label so that it goes into tomorrow's merge for review, or review it first separately in breaking?

@bpindelski
Copy link

One thing I noticed is the random appearance of

PersistentEventLogLoader.v2.current_id  |-1
pixelDataEventLogLoader.v1.current_id   |-1

in the configuration table. Once it appeared on the first start of the server, the other time - on the restart.

@bpindelski
Copy link

Besides the comment (#2893 (comment)), the code changes look OK. Good to merge.

@jballanc
Copy link
Contributor

jballanc commented Aug 1, 2014

@bpindelski As I recall, setting those values to "-1" is a signal to the indexing system that the entire event log needs to be reindexed. At least, that should be the case for PersistentEventLogLoader.v2.current_id. I'd have to look at the code again to say for sure if the same is true of pixelDataEventLogLoader.v1.current_id.

@mtbc
Copy link
Member Author

mtbc commented Aug 4, 2014

Thank you, @jballanc: yes, this PR oughtn't affect these *.current_id configuration keys.

@bpindelski
Copy link

@jballanc Thanks. In this case, the appearance of those rows doesn't cause any problems. Good to merge.

@mtbc
Copy link
Member Author

mtbc commented Aug 4, 2014

I suppose before merging, try a merge build without the breaking label?

@joshmoore joshmoore removed the breaking label Aug 4, 2014
@joshmoore
Copy link
Member

Definitely. breaking removed.

@ximenesuk
Copy link
Contributor

Does this need any further testing in the non-breaking build or is that it built good enough for merging?

@mtbc
Copy link
Member Author

mtbc commented Aug 5, 2014

I don't know, but somebody else will.

@joshmoore
Copy link
Member

I'm assuming based on the lack of explosions today on trout that this is good to go. Thanks, all.

joshmoore added a commit that referenced this pull request Aug 5, 2014
fix #970, #12317: adjust dbpatch table; delete map values with triggers
@joshmoore joshmoore merged commit 10f0f52 into ome:develop Aug 5, 2014
@mtbc mtbc deleted the trac-970-12317-dbpatch-and-map-value branch August 5, 2014 12:33
@sbesson sbesson added this to the 5.1.0-m1 milestone Oct 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants