You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/05/19 19:35:00 UTC
[GitHub] [accumulo] tchaie opened a new pull request, #2717: Create Public API to set multiple properties
tchaie opened a new pull request, #2717:
URL: https://github.com/apache/accumulo/pull/2717
Closes #2692
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] tchaie commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
tchaie commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1145277496
Will work on integrating the properties version as a parameter in modifyProperties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#discussion_r877502828
##########
core/src/main/thrift/manager.thrift:
##########
@@ -268,6 +268,15 @@ service ManagerClientService {
2:client.ThriftNotActiveServiceException tnase
)
+ void setSystemProperties(
+ 1:trace.TInfo tinfo
+ 2:security.TCredentials credentials
+ 3:map<string, string> propertiesMap
+ ) throws (
+ 1:client.ThriftSecurityException sec
+ 2:client.ThriftNotActiveServiceException tnase
+ )
+
Review Comment:
Thrift changes look good.
##########
server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java:
##########
@@ -68,6 +69,52 @@ public static void setSystemProperty(ServerContext context, String property, Str
context.getPropStore().putAll(SystemPropKey.of(context), Map.of(property, value));
}
+ public static void setSystemProperties(ServerContext context, Map<String,String> propertiesMap) {
+ Map<String,String> checkedProperties = new HashMap<>();
+
+ for (Map.Entry<String,String> entry : propertiesMap.entrySet()) {
+ final var original = entry.getKey();
+ var property = entry.getKey();
+ var value = entry.getValue();
+
+ // Retrieve the replacement name for this property, if there is one.
+ // Do this before we check if the name is a valid zookeeper name.
+ property = DeprecatedPropertyUtil.getReplacementName(property,
+ (log, replacement) -> log
+ .warn("{} was deprecated and will be removed in a future release;"
+ + " setting its replacement {} instead", original, replacement));
+
+ if (!Property.isValidZooPropertyKey(property)) {
+ IllegalArgumentException iae =
+ new IllegalArgumentException("Zookeeper property is not mutable: " + property);
+ log.debug("Attempted to set zookeeper property. It is not mutable", iae);
+ throw iae;
+ }
+
+ // Find the property taking prefix into account
+ Property foundProp = null;
+ for (Property prop : Property.values()) {
+ if (prop.getType() == PropertyType.PREFIX && property.startsWith(prop.getKey())
+ || prop.getKey().equals(property)) {
+ foundProp = prop;
+ break;
+ }
+ }
+
+ if ((foundProp == null || (foundProp.getType() != PropertyType.PREFIX
+ && !foundProp.getType().isValidFormat(value)))) {
+ IllegalArgumentException iae = new IllegalArgumentException(
+ "Ignoring property " + property + " it is either null or in an invalid format");
+ log.debug("Attempted to set zookeeper property. Value is either null or invalid", iae);
+ throw iae;
+ }
+
+ checkedProperties.put(property, value);
+ }
+
+ context.getPropStore().putAll(SystemPropKey.of(context), checkedProperties);
Review Comment:
This change may require updating the PropStore to replace the properties with these. Currently, `ZooPropStore.putAll` does a mutate of the existing properties to try to merge them into the existing properties. It does this by calling VersionedProperties.addOrUpdate. We probably need a new method on VersionedProperties, very similar to addOrUpdate, but doesn't initialize the new properties from the old ones. And then, that should be the method used by ZooPropStore in a new replaceAll (or setAll) method instead of calling putAll.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -47,6 +47,24 @@ public interface InstanceOperations {
void setProperty(final String property, final String value)
throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Sets multiple system properties in zookeeper. Tablet servers will pull this setting and
+ * override the equivalent setting in accumulo.properties. Changes can be seen using
+ * {@link #getSystemConfiguration()}.
+ * <p>
+ * Only some properties can be changed by this method, an IllegalArgumentException will be thrown
+ * if there is an attempt to set a read-only property.
+ *
+ * @param propertiesMap
+ * map containing key-value pairs of properties and corresponding values
+ * @throws AccumuloException
+ * if a general error occurs
+ * @throws AccumuloSecurityException
+ * if the user does not have permission
+ */
+ void setProperties(Map<String,String> propertiesMap)
Review Comment:
From the description of this new method, it's not clear if the set of properties will be merged with the existing properties that are set in ZooKeeper, or if they will completely replace them. If they are to be merged, then this API should also include the ability to mark a property to be deleted at the same time that other properties are set. So, I think it's probably easier to just make this map replace any existing map that is currently stored in ZooKeeper.
(In other words, if there was an existing override set, and it's not duplicated or overridden in this map here, then that previous override will be removed)
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java:
##########
@@ -74,6 +75,8 @@ public InstanceOperationsImpl(ClientContext context) {
@Override
public void setProperty(final String property, final String value)
throws AccumuloException, AccumuloSecurityException, IllegalArgumentException {
+ System.out.println("Property is " + property);
+ System.out.println("Value is " + value);
Review Comment:
I'm sure you only put these here for testing locally, but don't forget to remove these when you're done with developing this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1134616488
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1134933878
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1257912915
Yeah you can close it as done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1175118629
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1172948070
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1133596040
> 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:
```java
/**
* 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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#discussion_r877584349
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -47,6 +47,24 @@ public interface InstanceOperations {
void setProperty(final String property, final String value)
throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Sets multiple system properties in zookeeper. Tablet servers will pull this setting and
+ * override the equivalent setting in accumulo.properties. Changes can be seen using
+ * {@link #getSystemConfiguration()}.
+ * <p>
+ * Only some properties can be changed by this method, an IllegalArgumentException will be thrown
+ * if there is an attempt to set a read-only property.
+ *
+ * @param propertiesMap
+ * map containing key-value pairs of properties and corresponding values
+ * @throws AccumuloException
+ * if a general error occurs
+ * @throws AccumuloSecurityException
+ * if the user does not have permission
+ */
+ void setProperties(Map<String,String> propertiesMap)
Review Comment:
It may be easier to adopt the addOrUpdate semantics - there may be default properties that are added on table creation - and you probably do not want to remove them unless it is done explicitly.
I'm not sure how addOrUpdate handles empty (or null) values - but that could be used to remove a property as part of an atomic set group operation. Doing something like thatMap.of("a", "v1", "b", ", "c", "v2") - would either add or update a or c and remove b.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] tchaie commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
tchaie commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1132125598
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1133439498
> 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:
```java
client.setProperties(changeHoweverIWant(readCurrentProps()));
```
To make this explicit, the API interface could be:
```java
void setProperties(Function<Map<String,String>,Map<String,String>> oldPropsToNewProps);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1172959006
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#discussion_r878044667
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -47,6 +47,24 @@ public interface InstanceOperations {
void setProperty(final String property, final String value)
throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Sets multiple system properties in zookeeper. Tablet servers will pull this setting and
+ * override the equivalent setting in accumulo.properties. Changes can be seen using
+ * {@link #getSystemConfiguration()}.
+ * <p>
+ * Only some properties can be changed by this method, an IllegalArgumentException will be thrown
+ * if there is an attempt to set a read-only property.
+ *
+ * @param propertiesMap
+ * map containing key-value pairs of properties and corresponding values
+ * @throws AccumuloException
+ * if a general error occurs
+ * @throws AccumuloSecurityException
+ * if the user does not have permission
+ */
+ void setProperties(Map<String,String> propertiesMap)
Review Comment:
Maybe the API should accept a Map of properties to set, and a Set of properties to revert to default? That might make more intuitive than relying on interpreting null values. If a property name is in both the map and the set, the client can throw an IllegalArgumentException. On the server side, the current map could be mutated atomically by removing anything from the set, and then putting anything from the map, and storing the result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1133464577
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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1133136178
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
<details>
<summary>Click to expand!</summary>
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
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#discussion_r877585988
##########
server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java:
##########
@@ -68,6 +69,52 @@ public static void setSystemProperty(ServerContext context, String property, Str
context.getPropStore().putAll(SystemPropKey.of(context), Map.of(property, value));
}
+ public static void setSystemProperties(ServerContext context, Map<String,String> propertiesMap) {
+ Map<String,String> checkedProperties = new HashMap<>();
+
+ for (Map.Entry<String,String> entry : propertiesMap.entrySet()) {
+ final var original = entry.getKey();
+ var property = entry.getKey();
+ var value = entry.getValue();
+
+ // Retrieve the replacement name for this property, if there is one.
+ // Do this before we check if the name is a valid zookeeper name.
+ property = DeprecatedPropertyUtil.getReplacementName(property,
+ (log, replacement) -> log
+ .warn("{} was deprecated and will be removed in a future release;"
+ + " setting its replacement {} instead", original, replacement));
+
+ if (!Property.isValidZooPropertyKey(property)) {
+ IllegalArgumentException iae =
+ new IllegalArgumentException("Zookeeper property is not mutable: " + property);
+ log.debug("Attempted to set zookeeper property. It is not mutable", iae);
+ throw iae;
+ }
+
+ // Find the property taking prefix into account
+ Property foundProp = null;
+ for (Property prop : Property.values()) {
+ if (prop.getType() == PropertyType.PREFIX && property.startsWith(prop.getKey())
+ || prop.getKey().equals(property)) {
+ foundProp = prop;
+ break;
+ }
+ }
+
+ if ((foundProp == null || (foundProp.getType() != PropertyType.PREFIX
+ && !foundProp.getType().isValidFormat(value)))) {
+ IllegalArgumentException iae = new IllegalArgumentException(
+ "Ignoring property " + property + " it is either null or in an invalid format");
+ log.debug("Attempted to set zookeeper property. Value is either null or invalid", iae);
+ throw iae;
+ }
+
+ checkedProperties.put(property, value);
+ }
+
+ context.getPropStore().putAll(SystemPropKey.of(context), checkedProperties);
Review Comment:
See other comment concerning addOrUpdate instead of a replaceAll for set. replaceAll may be appropriate as a second, explicit method, but I think the ability to add or modify a subset should be available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1172946760
@cshannon thanks for the offer. You're welcome to pick it up where it was left off, if you wish.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1175114948
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1172960754
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#discussion_r878037133
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -47,6 +47,24 @@ public interface InstanceOperations {
void setProperty(final String property, final String value)
throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Sets multiple system properties in zookeeper. Tablet servers will pull this setting and
+ * override the equivalent setting in accumulo.properties. Changes can be seen using
+ * {@link #getSystemConfiguration()}.
+ * <p>
+ * Only some properties can be changed by this method, an IllegalArgumentException will be thrown
+ * if there is an attempt to set a read-only property.
+ *
+ * @param propertiesMap
+ * map containing key-value pairs of properties and corresponding values
+ * @throws AccumuloException
+ * if a general error occurs
+ * @throws AccumuloSecurityException
+ * if the user does not have permission
+ */
+ void setProperties(Map<String,String> propertiesMap)
Review Comment:
The three primitives are add, update, and remove. However, "add or update" is just "put" (or "set"), and "remove" isn't really a removal, but a "revert to default" (removal of any override set at this scope). I definitely want to include the ability to remove a property alongside the properties to set/update. I don't know if it's intuitive to rely on setting it to a null value, since that *looks like* it's setting an override value of `null`. I figured it was more intuitive to omit it from the map, and replace the entire map with the provided one, since having a full on Mutation API with `putDeletes` and such is a bit overkill for config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1172945021
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1257911701
@cshannon - we can close this now that #2799 is merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2717:
URL: https://github.com/apache/accumulo/pull/2717#issuecomment-1257914330
#2799 continued this work and was merged. Closing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion closed pull request #2717: Create Public API to set multiple properties
Posted by GitBox <gi...@apache.org>.
dlmarion closed pull request #2717: Create Public API to set multiple properties
URL: https://github.com/apache/accumulo/pull/2717
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org