Create Public API to set multiple properties

[tchaie]

Closes #2692

[tchaie]

WIP pending review and feedback on how to proceed. (Debugging statements will be removed when PR is ready to be finalized)

Created setProperties() functions, while keeping original setProperty() functions - is the intention to replace it completely or have some way to redirect setProperty() to the new function?

[EdColeman]

Maybe I'm missing something - but if setProperties replaces all properties except the ones set, someone would need to include multiple properties that are set by default. For the metadata table, that is 28 properties, for a user table, there are 7.

Using a vanilla uno instance the props look like:

sample uno default properties

Click to expand!

Tables: Name: accumulo.metadata, Data Version:2, Data Timestamp: 2022-05-20T16:43:25.07617Z: table.cache.block.enable=true table.cache.index.enable=true table.compaction.dispatcher=org.apache.accumulo.core.spi.compaction.SimpleCompactionDispatcher table.compaction.dispatcher.opts.service=meta table.compaction.major.ratio=1 table.constraint.1=org.apache.accumulo.server.constraints.MetadataConstraints table.durability=sync table.failures.ignore=false table.file.compress.blocksize=32K table.file.replication=5 table.group.server=file,log,srv,future table.group.tablet=~tab,loc table.groups.enabled=tablet,server table.iterator.majc.bulkLoadFilter=20,org.apache.accumulo.server.iterators.MetadataBulkLoadFilter table.iterator.majc.replcombiner=9,org.apache.accumulo.server.replication.StatusCombiner table.iterator.majc.replcombiner.opt.columns=stat table.iterator.majc.vers=10,org.apache.accumulo.core.iterators.user.VersioningIterator table.iterator.majc.vers.opt.maxVersions=1 table.iterator.minc.replcombiner=9,org.apache.accumulo.server.replication.StatusCombiner table.iterator.minc.replcombiner.opt.columns=stat table.iterator.minc.vers=10,org.apache.accumulo.core.iterators.user.VersioningIterator table.iterator.minc.vers.opt.maxVersions=1 table.iterator.scan.replcombiner=9,org.apache.accumulo.server.replication.StatusCombiner table.iterator.scan.replcombiner.opt.columns=stat table.iterator.scan.vers=10,org.apache.accumulo.core.iterators.user.VersioningIterator table.iterator.scan.vers.opt.maxVersions=1 table.security.scan.visibility.default= table.split.threshold=64M

Name: tbl1, Data Version:1, Data Timestamp: 2022-05-20T16:50:47.568659Z: table.constraint.1=org.apache.accumulo.core.data.constraints.DefaultKeySizeConstraint table.iterator.majc.vers=20,org.apache.accumulo.core.iterators.user.VersioningIterator table.iterator.majc.vers.opt.maxVersions=1 table.iterator.minc.vers=20,org.apache.accumulo.core.iterators.user.VersioningIterator table.iterator.minc.vers.opt.maxVersions=1 table.iterator.scan.vers=20,org.apache.accumulo.core.iterators.user.VersioningIterator table.iterator.scan.vers.opt.maxVersions=1

[ctubbsii]

Maybe I'm missing something - but if setProperties replaces all properties except the ones set, someone would need to include multiple properties that are set by default. For the metadata table, that is 28 properties, for a user table, there are 7.

The idea was that you've now empowered the user to read the existing properties, keep all, none, or some of the ones they want, do any updates on the client side, and then atomically set the result. It's a very simple, primitive API that enables the user to do whatever they want. Yes, if they want to keep any default ones, they would have to explicitly preserve those. But, it's not like the number of these is a problem... you can do this programmatically pretty trivially:

client.setProperties(changeHoweverIWant(readCurrentProps()));

To make this explicit, the API interface could be:

void setProperties(Function<Map<String,String>,Map<String,String>> oldPropsToNewProps);

[EdColeman]

Maybe I am too focused on how it would be handled via the shell if we wanted to allow multiple, atomic sets there.

And your example is great - we should capture that in the in the seems more explicit in the

void setProperties(Function<Map<String,String>,Map<String,String>> oldPropsToNewProps);

[ctubbsii]

Maybe I am too focused on how it would be handled via the shell if we wanted to allow multiple, atomic sets there.

Mutating the props in the shell would be a huge pain. It'd be easier to use bin/accumulo jshell and work directly on the map in there.

Also, I'm thinking Consumer would be better than Function:

/**
 * Modify system properties using a Consumer that accepts a mutable map containing the current system
 * property overrides stored in ZooKeeper. If the supplied Consumer alters the map without throwing an
 * Exception, then the resulting map will atomically replace the current system property overrides in
 * ZooKeeper. Only properties which can be stored in ZooKeeper will be accepted.
 *
 * @throws IllegalArgumentException if the Consumer alters the map by adding properties that cannot be
 *         stored in ZooKeeper
 * @throws ConcurrentModificationException without altering the stored properties if the server reports
 *         that the properties have been changed by another process
 */
void modifyProperties(Consumer<Map<String,String>> mapMutator);

[milleruntime]

Instead of adding a method to InstanceOperations what about adding a static method to the new Accumulo entry point? We could develop a fluent API, similar to what we did with the newClientProperties() method.

[ctubbsii]

Instead of adding a method to InstanceOperations what about adding a static method to the new Accumulo entry point? We could develop a fluent API, similar to what we did with the newClientProperties() method.

A full on fluent API for mutating/building system properties is a bit overkill. It was why I was originally thinking we'd want to just support a primitive replace operation. However, with the Consumer idea, we can get all the benefits of having a full-fledged mutate API, but all in a single method, without any bloat. As for the static method, with a separate entry point, I'm not sure that would be helpful at all, because the properties are already tied to an instance, like the current setProperty and removeProperty are. After this, we're also going to want to add similar features for table and namespace operations. So, I don't think it makes sense to put the entry point for this as a static method on the Accumulo entry point.

[tchaie]

Will work on integrating the properties version as a parameter in modifyProperties. (This is why the Pair code is currently commented out in InstanceOperationsImpl)

[cshannon]

@ctubbsii and @EdColeman - What do you think of the current state of this? It looks like this is almost done but needs some clean up and some tests, etc. I can take this over and wrap it up.

[ctubbsii]

@cshannon thanks for the offer. You're welcome to pick it up where it was left off, if you wish.

[cshannon]

@cshannon thanks for the offer. You're welcome to pick it up where it was left off, if you wish.

Sounds good, I'll start working on an updated PR.

[cshannon]

Looks like this actually still needs some more work as it only address InstanceOperations and not NamespaceOperations and TableOperations so I'm working on cleaning up the code and then adding those implementations and tests.

@ctubbsii - I'm going through the history of the PR and just wanted to get one clarification and make sure I'm on the same page. It seems like based on the comments the intent here of the modifyProperties(mapMutator) is to also delete and not just modify/add to the existing properties. This would imply any properties that currently exist in the config (for the given PropStoreKey) but are no longer included in the mutated map after the mapMutator handles the properties map should be deleted which makes sense to me. Correct?

[ctubbsii]

Yes

[cshannon]

@ctubbsii - I am going to open a new pull request when I am finished with my changes. How do you want me to handle the history here? Should I keep the original commits from @tchaie or just squash her commits into 1 and then add my new commit after, or just do 1 commit etc? I'm currently working off a rebased version of this branch.

Also looks like I will want to make sure to rebase and fix things after #2796 is merged

[ctubbsii]

@ctubbsii - I am going to open a new pull request when I am finished with my changes. How do you want me to handle the history here? Should I keep the original commits from @tchaie or just squash her commits into 1 and then add my new commit after, or just do 1 commit etc? I'm currently working off a rebased version of this branch.

Also looks like I will want to make sure to rebase and fix things after #2796 is merged

Squashing the original commits to 1, and having your commits after will be best. It'll be the easiest to work with, for any rebasing.

[dlmarion]

@cshannon - we can close this now that #2799 is merged?

[cshannon]

Yeah you can close it as done.

[dlmarion]

#2799 continued this work and was merged. Closing.