Fixed #312: added support for group and host properties, backwards compatible

[grtjn]

No description provided.

[grtjn]

Weird it doesn't allow automatic merge. Maybe just too many changes, even though they are relatively straight-forward..

[grtjn]

The changes are such that existing ml-configs are automatically converted to the required wrapping of app-servers and such into group elements. It leverages the already supported group attributes if present..

[grtjn]

Ah wait, another pull request has been opened while I was working on this one.. :-/

[dmcassel]

Can you rebase?

[grtjn]

Worth a try for sure, but I guess you'd have to merge the other pull request (#318) first. I think we both touch the same are in server_config.rb..

[dmcassel]

Just merged #318, please try rebasing now.

[grtjn]

Rebased, and pushed again. Saw some extra changes related to external security. Probably those were missing at my end, hence the non-automerge. Resolved now..

Doing some extra tests first though, please hold off from merging for the moment..

[grtjn]

Ready to role! Anyone up for review? :)

[grtjn]

While running self-test myself I noticed it kind of conflicted with how the original group functionality was working. Or to put it more accurately, I added some assertions that revealed an omission in the original functionality.

The thing is that there must be at least one host in a group for servers of that group to be useful, and to allow tying forests to hosts within that group. The new functionality derives all referenced group-names from the config. Note that anything without a group attribute was dedicated to the Default group, which now can be overridden with default-group attribute on the root element. It then verifies whether the config will assign hosts to a group, or hosts within that group are already present.

The current self-test code creates a group called roxy-self-test, but does not assign hosts to it, as there was no functionality to do so. You could manually create that group, assign a host, but that would not survive wipe. Host would be reverted to Default, as soon as the group gets deleted.

Another issue was that some parts of the ml-config are being generated, so you cannot add a group attribute to them. That is where I added the default-group feature for.

Anyhow, I ran the self-test against ML7 and ML6, both with cluster and non-cluster. They ran successfully. Anyone care to review?

[dmcassel]

are these tabs? please replace with spaces for consistency

disregard, looks like they are spaces.

[grtjn]

I believe I indented large parts of bootstrap and wipe code, because of extra try/catch and/or rewriting of config..

[paxtonhare]

Looks pretty awesome. I like what you've done. The rewrite config part is nice. I'm wondering if it makes sense to go ahead and commit to the "new" format in the sample ml-config.xml file rather than relying on the rewrite-config(). I'd like to hear @grtjn and @dmcassel 's opinions on this.

[dmcassel]

If you're expecting ml.group to be defined, you should add it to default.properties with a default value. That file is part of the upgrade process.

[grtjn]

Already in default.properties..

[dmcassel]

@paxtonhare @grtjn I'm okay with a new starting format for ml-config.xml, as long as the code still works for existing ml-config.xmls. Need to maintain backwards compatibility, and I think that means we'll need to keep the rewrite-config() stuff (right?).

[paxtonhare]

@dmcassel we definitely need to keep the rewrite-config(). And you just said what I meant to say. I like the idea of doing the new format but keeping backwards compat.

[dmcassel]

Cool. +1 on the merge, with @grtjn to file a ticket to add min-version on group settings.

[grtjn]

About ml.group: I believe it is already in default.properties with default value Default (lot of default :))

About min-version: mostly for future proof, in case ML8 is introducing settings not supported in older versions. I ran the current set against ML6 and ML7, no issues there. Maybe create a ticket to explicitly check against ML5?

[grtjn]

About rewriting sample config: what would be a sensible set of group settings to expose by default?

It would be nice if all supported settings would be listed somewhere, possibly with supported min-versions? Maybe something that can be generated out of those settings variables within setup.xqy?

[dmcassel]

Does the performance-metering stuff go back to ML6? I don't think it goes back to ML5. ML5 hits EOL months from now, maybe just deal with bugs as they are reported. If we're confident everything works back to ML6, I'm good with that.

[paxtonhare]

@grtjn for default settings of group... You can just omit them and it will use defaults. So just do a barebones wrapper with the name.

[grtjn]

Metering, good question. It does have monitoring, but not the history. Nor does it have a Metering database. Weird, didn't seem to complain though.

@paxtonhare, can you easily (self-)test against ML6? (as part of #325 perhaps)

[grtjn]

@paxtonhare Can you elaborate what you meant with barebones wrapper?

[paxtonhare]

<groups xmlns="http://marklogic.com/xdmp/group" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://marklogic.com/xdmp/group group.xsd">
  <group>
    <group-name>Default</group-name>
    <http-servers>....
  </group>
</groups>

Just include the name. Nothing else.

[grtjn]

So, no configuration/@default-group, no server/@group, no database/@group, and no hosts? I personally think would be nice to leave some of that in, with a reference to @ml.group property. Only catch is that @ml.bootstrap-host (which refers to the hostname within MarkLogic) is not defined yet. You need that to insert at least one example host. I only defined that property in the self-test code. Would need to be copied over to server_config.rb..

[grtjn]

I have pushed another change for the sample ml-config. I dropped group attributes, but had to preserve default-group, as that applies to databases.

I also removed the group settings as suggested, though I still think it would be nice to have some kind of reference ready at hand (also applies to database settings etc, I am often looking in the code for the correct name of a setting). Having them around in comments would help a bit, but it also clutters. Let me open a new ticket for that..

[grtjn]

Is the PR agreeable like this?

[dmcassel]

good by me. @paxtonhare ?