Allow removal of global configuration properties

[cdegroc]

Fixes #555

Usage

mgmt = graph.openManagement()
mgmt.set(<config_key>, null)
mgmt.commit()

I've confirmed locally that, once removed, configuration keys can't be retrieved using ManagementSystem#get() and don't appear anymore in graph.getBackend().getGlobalSystemConfig().

---

Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you to ensure the following steps have been taken:

For all changes:

• [X] Is there an issue associated with this PR? Is it referenced in the commit message?
• [X] Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
• [X] Has your PR been rebased against the latest commit within the target branch (typically master)?
• [X] Is your initial contribution a single, squashed commit?

For code changes:

• [X] Have you written and/or updated unit tests to verify your changes?
• [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• [ ] If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
• [ ] If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

• [ ] Have you ensured that format looks appropriate for the output in which it is rendered?

[li-boxuan]

How will JanusGraph behave if an index backend is configured and in use for some mixed index and the index backend is then removed from the config? Will it still try to use it for traversals that could use that index?

This definitely needs experiments (or even better, having a test case to showcase that), but AFAIK this will cause read & writes involving that indexing backend to fail. The reason is that schema information is stored in the storage backend, not in the index backend. So JanusGraph will assume the mixed indexes still exist and try connecting to the indexing backend.

I think it could be a good improvement to prevent users from removing an index backend in config if there is an active mixed index for that index backend.

[cdegroc]

How will JanusGraph behave if an index backend is configured and in use for some mixed index and the index backend is then removed from the config? Will it still try to use it for traversals that could use that index?

Agreed. We should add tests and update the documentation.

Nonetheless, in my understanding, this change is not specific to index backends (it allows removal of any configuration property). In addition, removing an index backend can already be done when using the ConfiguredGraphFactory as shown in this test: https://github.com/cdegroc/janusgraph/commit/e37c9b4736f404545495b518f41fb01391cebc7d. Though, to be fair, I don't think this is documented in the ConfiguredGraphFactory doc.

[cdegroc]

Rather than this breaking change, add a remove API and add some Javadoc saying that removal of config might not work properly under all circumstances (e.g. index settings) so use it at your own risk.

After all, it is clear that this functionality is useful in many cases, so some advanced users could still utilize it. We don't want to mention it in the user doc until we have figured out and documented all edge cases.

Thanks @li-boxuan. Adding a remove API would be cleaner and the "use at your own risk" approach sounds right to me.

The disadvantage is that this API will be different from ConfiguredGraphFactory, but we could in turn update ConfiguredGraphFactory to have a similar API (for instance, add a removeConfiguration(String graphName, Configuration config) method.

One issue is that Configuration are key/value collections and so we would have to use removeConfiguration(String graphName, Iterable<String> configKeys).

[li-boxuan]

The disadvantage is that this API will be different from ConfiguredGraphFactory, but we could in turn update ConfiguredGraphFactory to have a similar API (for instance, add a removeConfiguration(String graphName, Configuration config) method

That sounds good to me. @FlorianHockmann what do you think?

[FlorianHockmann]

That sounds good to me. @FlorianHockmann what do you think?

Sure, sounds good to me! :+1:

[FlorianHockmann]

Just stumbled upon this PR again when looking into the open issues for the 0.6.3 milestone. It's not clear to me any more what we agreed on here exactly 🙁

If I understand our discussion here correctly then what's mostly missing is some addition to the Java docs that explains how this removal works but also warns that it won't work for all options. Is that correct? And @cdegroc, are you planning to come back to this PR any time soon? Then we can include it in the 0.6.3 release.

If you come back to this PR, then it probably also makes sense to change the target branch to master and add the labels backport and backport/v0.6. We have recently changed our merging strategy: Instead of requiring contributors to target v0.6 and then merge the PR first into v0.6 and then merge that into master, we now only target master and let a bot create a separate backporting PR if we want a change to also land into v0.6. But we can alternatively probably also merge this into v0.6 and then cherry-pick the commit into master.

This seems to be still open:

The disadvantage is that this API will be different from ConfiguredGraphFactory, but we could in turn update ConfiguredGraphFactory to have a similar API (for instance, add a removeConfiguration(String graphName, Configuration config) method.

Do we want to do this as part of this PR or in a follow-up one? Doing it in a different PR is OK to me, but could you please create an issue for that, @cdegroc? (I think you have a better understanding of this to properly explain what needs to be done.)

[porunov]

To me it looks like a good feature to have but not a critical bug. Thus, I propose to reschedule this PR to v0.6.4 or 1.0.0. And start 0.6.3 release process.

[li-boxuan]

I would like to have this PR in 1.0.0, so I am inclined to merge it. We could add an one-liner in release notes that it's a breaking change. Since it's a breaking change, maybe we shouldn't put it in 0.6.4, but I don't have a strong opinion yet.

[cdegroc]

👋 Sorry for not answering earlier. I'll try to catch up and see if there's anything I can help with.

[cdegroc]

Created #4061 to address comments regarding the interface. Let me know what you think.