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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
