keith-turner commented on code in PR #5781:
URL: https://github.com/apache/accumulo/pull/5781#discussion_r2260608150
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java:
##########
@@ -1069,4 +1070,11 @@ default Stream<TabletInformation>
getTabletInformation(final String tableName, f
throw new UnsupportedOperationException();
}
+ /**
+ * Returns the namespace for the given table name
+ *
+ * @param table fully qualified table name
+ * @return namespace id
Review Comment:
Couid document and test what runtime exceptions this could throw.
##########
test/src/main/java/org/apache/accumulo/test/NamespacesIT_SimpleSuite.java:
##########
@@ -871,78 +870,6 @@ public void testPermissions() throws Exception {
}
}
- @Test
- public void verifySystemPropertyInheritance() throws Exception {
-
- try (AccumuloClient client =
- getCluster().createAccumuloClient(getPrincipal(), new
PasswordToken(getRootPassword()))) {
- client.securityOperations().grantNamespacePermission(getPrincipal(),
"accumulo",
- NamespacePermission.ALTER_NAMESPACE);
- client.securityOperations().grantNamespacePermission(getPrincipal(), "",
- NamespacePermission.ALTER_NAMESPACE);
- }
-
- String t1 = "1";
- String t2 = namespace + "." + t1;
- c.tableOperations().create(t1);
- c.namespaceOperations().create(namespace);
- c.tableOperations().create(t2);
-
- // verify iterator inheritance
- _verifySystemPropertyInheritance(t1, t2,
Property.TABLE_ITERATOR_PREFIX.getKey() + "scan.sum",
Review Comment:
May want to keep this test w/ different non table properties. Will still
have inheritance so its nice to test that.
##########
test/src/main/java/org/apache/accumulo/test/conf/util/ZooPropEditorIT_SimpleSuite.java:
##########
@@ -67,13 +70,18 @@ public void modifyPropTest() throws Exception {
LOG.debug("Tables: {}", client.tableOperations().list());
- // override default in sys, and then over-ride that for table prop
-
client.instanceOperations().setProperty(Property.TABLE_BLOOM_ENABLED.getKey(),
"true");
+ // override default in namespace, and then over-ride that for table prop
+ AccumuloException ae = assertThrows(AccumuloException.class, () ->
client.instanceOperations()
Review Comment:
This is a super important test. May be worthwhile to move it to its own
test or duplicate this in its own test to make it less likely to be lost in
test refactoring. Its kinda hidden away in a test of other things.
In a stand alone test could try setting more table props and maybe some
table prefix type props too.
##########
test/src/main/java/org/apache/accumulo/test/conf/PropStoreConfigIT_SimpleSuite.java:
##########
@@ -97,23 +97,32 @@ public void setTablePropTest() throws Exception {
try (var client = Accumulo.newClient().from(getClientProps()).build()) {
client.tableOperations().create(table);
+ NamespaceId nid = client.tableOperations().getNamespace(table);
log.debug("Tables: {}", client.tableOperations().list());
- // override default in sys, and then over-ride that for table prop
-
client.instanceOperations().setProperty(Property.TABLE_BLOOM_ENABLED.getKey(),
"true");
+ // override default in namespace, and then over-ride that for table prop
Review Comment:
could move this comment down a few lines
##########
server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java:
##########
@@ -89,8 +89,10 @@ private static String validateSystemProperty(ServerContext
context, SystemPropKe
log.trace("Encountered error setting zookeeper property", iae);
throw iae;
}
- if (Property.isValidTablePropertyKey(property)) {
- PropUtil.validateProperties(context, key, Map.of(property, value));
+ if (property.startsWith(Property.TABLE_PREFIX.getKey())) {
+ throw new IllegalArgumentException(
+ "Table property " + property + " cannot be set at the system level."
+ + " Set table properties at the namespace or table level.");
Review Comment:
Would be nice to somehow consolidate this exception message between the two
places. Maybe could move this into a method in PropUtil, like
PropUtil.checkForTablePropAtSystemLevel(property). That method does the prefix
check and throws the exception and both places can call it.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java:
##########
@@ -1069,4 +1070,11 @@ default Stream<TabletInformation>
getTabletInformation(final String tableName, f
throw new UnsupportedOperationException();
}
+ /**
+ * Returns the namespace for the given table name
+ *
+ * @param table fully qualified table name
+ * @return namespace id
+ */
+ NamespaceId getNamespace(String table);
Review Comment:
Needs a since tag
--
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]
