You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "kevinrr888 (via GitHub)" <gi...@apache.org> on 2023/12/28 17:26:06 UTC
[PR] Return non-merged view of System config [accumulo]
kevinrr888 opened a new pull request, #4120:
URL: https://github.com/apache/accumulo/pull/4120
Closes https://github.com/apache/accumulo/issues/4095
- Added getSystemProperties() to InstanceOperations which functions similary to getTableProperties() and getNamespaceProperties() from TableOperations and NamespaceOperations.
- Added its implementation in InstanceOperationsImpl.
- Added a test getSystemPropertiesTest() to PropStoreConfigIT to test that the new method functions as intended.
--
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
Re: [PR] Return non-merged view of System config [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #4120:
URL: https://github.com/apache/accumulo/pull/4120
--
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
Re: [PR] Return non-merged view of System config [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #4120:
URL: https://github.com/apache/accumulo/pull/4120#discussion_r1441883289
##########
test/src/main/java/org/apache/accumulo/test/conf/PropStoreConfigIT.java:
##########
@@ -330,6 +330,37 @@ public void modifyInstancePropertiesTest() throws Exception {
}
}
+ @Test
+ public void getSystemPropertiesTest() throws Exception {
+
+ // Tests that getSystemProperties() does not return a merged view from its parent configuration
+ // (only returns those set at the system level)
+
+ String customPropKey1 = "table.custom.prop1";
+ String customPropKey2 = "table.custom.prop2";
+ String customPropVal1 = "v1";
+ String customPropVal2 = "v2";
+ try (var client = Accumulo.newClient().from(getClientProps()).build()) {
+ // non-merged view
+ Map<String,String> sysProps = client.instanceOperations().getSystemProperties();
+ // merged view
+ Map<String,String> sysConfig = client.instanceOperations().getSystemConfiguration();
+ assertEquals(0, sysProps.size());
+ assertFalse(sysConfig.isEmpty());
+ client.instanceOperations().setProperty(customPropKey1, customPropVal1);
+ client.instanceOperations().setProperty(customPropKey2, customPropVal2);
+ sysProps = client.instanceOperations().getSystemProperties();
Review Comment:
You may want to combine the assignment and test for expected size in Wait.waitFor() - the propagation of property changes occur asynchronously through ZooKeeper and when running in a constrained environment (like the CI environment) can cause the test to fail if the set and then checks occur before the update is propagated.
Testing for the size = expected, or wait a short period and then retry should be sufficient and then the values could be checked for the expected values.
--
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
Re: [PR] Return non-merged view of System config [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4120:
URL: https://github.com/apache/accumulo/pull/4120#discussion_r1441145351
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -166,6 +166,15 @@ Map<String,String> modifyProperties(Consumer<Map<String,String>> mapMutator)
*/
Map<String,String> getSiteConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieve a map of the system properties set in Zookeeper. Note that this does not return a
+ * merged view of the properties from its parent configuration. See
+ * {@link #getSystemConfiguration} for a merged view.
+ *
+ * @return A map of the system properties set in Zookeeper only.
Review Comment:
This will need 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return non-merged view of System config [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on PR #4120:
URL: https://github.com/apache/accumulo/pull/4120#issuecomment-1877574411
Added since 3.1 tag to `getSystemProperties()` and added `Wait.waitFor()` in `getSystemPropertiesTest()`
--
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