Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Fixing Issue #850: bootstrap broken in ML 8.0-7 #852

Conversation

joecrean
Copy link

@joecrean joecrean commented Sep 28, 2017

Since the upgrade to MarkLogic 8.0-7 roxy bootstrap fails in an
environment where the application was already deployed (and by converse
it does not fail in a fresh environment).

Path namespaces are removed and then added every time bootstrap runs.
In order to remove them we first need to check if they are being used.
They can be used in Path Range Index definitions, elements containing
Geospatial co-ordinates and Field definitions. The API used to delete
the path namespaces is called admin:database-delete-path-namespace.
This API will check to see if the namespaces are in use before
deletion and in 8.0-7 this has been tightened up to additionally
check field definitions.

Up to now roxy has simply deleted and re-added ALL path range indexes
and their namespaces which in itself could cause lots of re-indexing.
Now with the stricter API we would need to delete and re-add field
range indexes. This is clearly a bad direction and for that reason
this solution, detects what has changed and only removes indexes when
it is really necessary. #850

@grtjn grtjn changed the base branch from master to dev September 28, 2017 13:01
Since the upgrade to MarkLogic 8.0-7 roxy bootstrap fails in an
environment where the application was already deployed (and by converse
it does not fail in a fresh environment).

Path namespaces are removed and then added every time bootstrap runs.
In order to remove them we first need to check if they are being used.
They can be used in Path Range Index definitions, elements containing
Geospatial co-ordinates and Field definitions. The API used to delete
the path namespaces is called admin:database-delete-path-namespace.
This API will check to see if the namespaces are in use before
deletion and in 8.0-7 this has been tightened up to additionally
check field definitions.

Up to now roxy has simply deleted and re-added ALL path range indexes
and their namespaces which in itself could cause lots of re-indexing.
Now with the stricter API we would need to delete and re-add field
range indexes. This is clearly a bad direction and for that reason
this solution, detects what has changed and only removes indexes when
it is really necessary.
@joecrean joecrean force-pushed the issue-850-bootstrap-broken-path-namespaces branch from b5abedf to bb6ad35 Compare September 28, 2017 13:24
@grtjn
Copy link
Contributor

grtjn commented Sep 28, 2017

Thnx! Looks good at quick glance..

@RobertSzkutak
Copy link
Contributor

A major customer is being impacted by this. Will perform a review...

@RobertSzkutak
Copy link
Contributor

RobertSzkutak commented Oct 20, 2017

Overall this looks fine to me. Once concern I have is that this doesnt address these new geospatial indexes in ML9 : https://docs.marklogic.com/admin:database-get-geospatial-region-path-indexes

So presumably this would drop paths being used exclusively by these indexes which I imagine could cause issues. On the other hand, we don't support creating these indexes in Roxy yet either so...

@joecrean
Copy link
Author

Hi @RobertSzkutak sorry it took a while to get back to you on this. You are right I missed out on the above - that is because I am working with ML 8 so i didn't have any visibility on that new API. In any case if we enhance to cover it we will have to ensure the we also test for version 9 as well. I can build this in if you want..

@grtjn
Copy link
Contributor

grtjn commented Dec 1, 2017

Related to #863 too..

@grtjn grtjn mentioned this pull request Dec 1, 2017
) as element(configuration)
{
(:paths were not supported before this version of ML:)
if (setup:at-least-version("6.0-2")) then
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is redundant. ML7- is obsolete, and below references functions that wouldn't exist in ML6, so setup.xqy wouldn't compile at all..

Copy link
Contributor

Choose a reason for hiding this comment

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

Then again, minor detail..

@grtjn
Copy link
Contributor

grtjn commented Jan 9, 2018

I think the general idea is good. Rather than delete-all, create-all on path-namespaces, it is better to only remove unneeded, and add new ones instead.

I am less fond of deleting indexes as part of apply-path-namespaces though. I think it would be better to delete unneeded indexes first, then only delete/create namespaces as needed, and add new indexes. That would be a cleaner approach i think.

Good to see the abundant use of maps, iterator operator, and function invocation, but not sure it makes the code easier to read, particularly since most other code uses old-fashion style xqy.

I'll try to open an alternative PR later this week, and see if I can include the changes of #864 in that as well..

@grtjn grtjn self-assigned this Jan 10, 2018
@grtjn grtjn added this to the January 2018 milestone Jan 10, 2018
@grtjn
Copy link
Contributor

grtjn commented Jan 10, 2018

People have been waiting a while for this, sorry for the long delay! There were some PR's from @tdiepenbrock that interfered with this, and actually solved the same issue, while adding enhancements at the same time.

I replaced this PR with PR #866, which should effectively do the same, and adds support for metadata fields and the relatively new geo path indexes. Thanks very much @joecrean. I ended up choosing Tom's code over yours, but it was definitely useful nonetheless. It helped straighten thoughts, and focus on the issue. I couldn't have produced my PR without you and Tom's help this quickly..

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

Successfully merging this pull request may close these issues.

4 participants