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/09/30 11:14:25 UTC
[GitHub] [accumulo] cshannon opened a new pull request, #2986: Re-add getProperties() to InstanceOperationsApi to InstanceOperations
cshannon opened a new pull request, #2986:
URL: https://github.com/apache/accumulo/pull/2986
This method was removed in #2985 because it returned a Thrift object and appeared to be return the same properties as getSystemConfiguration(). This commit readds the method back (but removes the Thrift object) as it only returns the properties set in ZK which is different than getSystemConfiguration() which also returns defaults.
This also fixes the test failure in PropStoreConfigIT
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#issuecomment-1263499527
If you are going to add a new method, why not give the user more control with something like:
<pre>
/**
* Get specific set of system configuration properties.
*/
Map<String,String> getSystemConfiguration(ConfigurationType type);
</pre>
We already have a thrift type for `ConfigurationType` so it could be new API Enum:
<pre>
enum ConfigurationType {
CURRENT, SITE, DEFAULT
}
</pre>
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984552973
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
1) If I have shell access that is different from needing access to read files on a system.
2) There is a hash check that checks properties have the same critical values so there is a good probability that an active tserver has properties that match across the cluster.
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984517348
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
Sounds good, we can go back to the old name if everyone agrees. I would only want to change it if there is agreement which there is not. I won't make changes yet until we get everyone on the same page and then we can do another update and merge it.
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#issuecomment-1263649411
> @ctubbsii - Anything we can do here to fix the history after my mistake (squash the 3 commits back to 1) or at this point is it too late since it's in main?
We should leave the history as-is, since the commits have already been pushed.
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984602487
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
Based on the discussion not being settled for now I think it's best to just fix the test so I pushed a commit to do that. We could create another follow on Issue to discuss about changing the API and what methods should be exposed on InstanceOperations if we want to do that.
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984534728
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
So, I looked at NamespaceOperations and TableOperations. Both of them have a `getProperties` method that returns properties scoped at the appropriate level. On InstanceOperations we have:
getSystemConfiguration() -> returns the entire running configuration
getSiteConfiguration() -> returns the properties set in `accumulo.properties`
getSystemProperties() -> we're proposing to resurrect this method because of a failed test, which would return only the properties stored in ZooKeeper.
Why would a user, via the AccumuloClient, need to know where the properties are stored? Why should they care? I can see this is a server-side capability, but why does it exist in the client API? I would propose the following:
1. that we modify InstanceOperations to have a `getProperties` method that returns properties at the instance level just like NamespaceOperations and TableOperations.
2. That we deprecate (and remove) getSiteConfiguration and getSystemProperties in InstanceOperations. There is no reason that PropStoreConfigIT can't use a server-side API to get the information that it needs for the test.
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984515238
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
Again, the word `stored` here does not convey how these properties are different than the ones returned by `getSystemConfiguration`. The user will need to look at the documentation to know, so IMO we should leave the name as `getSystemProperties`.
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#issuecomment-1263478206
Is there a use case for this method? Or is it just to return the same properties set by the new 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] EdColeman commented on pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#issuecomment-1263487687
I ran the `PropStoreConfigIT` test with these changes and it now passes
--
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 #2986: Re-add getProperties() to InstanceOperationsApi to InstanceOperations
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984485562
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the configured System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the configured properties in zookeeper
+ * and not properties from accumulo.properties or defaults.
+ *
+ * @return a map of system properties set in zookeeper
+ */
+ Map<String,String> getSystemProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
Would renaming this to something like `getStoredProperties` help in highlighting the difference between the methods?
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperationsApi to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984487416
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the configured System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the configured properties in zookeeper
+ * and not properties from accumulo.properties or defaults.
+ *
+ * @return a map of system properties set in zookeeper
+ */
+ Map<String,String> getSystemProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
Sure, we can rename it to something else if it makes it easier to distinguish the the difference as there was confusion around it. I think it's important to expose the method because with the new API you can mutate multiple properties at once and it's going to be very useful to query what properties were set vs what are defaults so the user can know what's been changed, etc.
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984576775
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
> If I have shell access that is different from needing access to read files on a system.
> There is a hash check that checks properties have the same critical values so there is a good probability that an active tserver has properties that match across the cluster.
True statements, but you didn't address the why. I still don't understand the utility here.
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984548912
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
Why would the user need to know what is set in `accumulo.properties` vs what is stored in `ZooKeeper`. `InstanceOperations.getSiteConfiguration` connects to a random tablet server to get the configuration on that server. The `accumulo.properties` file on that server may be different than other servers.
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#issuecomment-1263620266
Ah I was trying to rebase/squash but must have just picked the rebase only option as all 3 commits showed up. Sorry about that, I guess it's too late to fix as we obviously don't want to force push on main.
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#issuecomment-1263618436
Oops, I thought it would pop up with an option to amend the commit message but I didn't see it so the commit message is wrong but this is obviously been rebased and 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] cshannon commented on a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984517348
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
Sounds good, we can go back to the old name. I would only want to change it if there is agreement which there is not.
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#issuecomment-1263481085
> Is there a use case for this method? Or is it just to return the same properties set by the new method?
The use case is getting back what properties were set in ZK explicitly which is going to be useful when making configuration changes. The other method returns the entire configuration so you get back a couple hundred properties but most are going to be either values stored in the properties file or defaults. This allows you to find out exactly what was overridden so can be used to mutate using the new modify properties api.
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984524252
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
I dunno, the term "override" could be confusing too because in the shell we also use that for site config (accumulo.properties) and ZK properties. For example, if I set "tserver.wal.max.size", the shell displays:
<pre>
default | tserver.wal.max.size ............................................. | 1G
site | @override ..................................................... | 512M
system | @override ..................................................... | 400M
</pre>
--
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 merged pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon merged PR #2986:
URL: https://github.com/apache/accumulo/pull/2986
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#issuecomment-1263630290
> Is the functionality of the atomic updates covered in unit tests? If that coverage is sufficient, then this test may not be adding value and it seems odd to need to add thrift dependencies in an IT test.
>
> I would prefer that the test find another way to test this functionality without a direct thrift call if at all feasible.
I don't see why we can't use thrift methods in the IT test but I just wanted to get the build working again since the test was broken. We could open up another issue to discuss the API changes around this further now that the test is fixed.
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#issuecomment-1263641673
@ctubbsii - Anything we can do here to fix the history after my mistake (squash the 3 commits back to 1) or at this point is it too late since it's in main?
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984519851
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
Stored or something equivalent seems to point in the correct direction - they would need to look at the documentation to find out what the difference between `getSystemConfiguration` and `getSystemProperies` difference is anyway - not having it named properties in the code (too me) increases clarity because properties and configuration seem to be easily confused
--
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 #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984540986
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
For instance operations, `getSiteConfiguration` seems to still makes sense to fetch what is set via the configuration.
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984510630
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the configured System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the configured properties in zookeeper
+ * and not properties from accumulo.properties or defaults.
+ *
+ * @return a map of system properties set in zookeeper
+ */
+ Map<String,String> getSystemProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
I'd rather keep `getSystemProperties` vs using `getStoredProperties`. If someone wants to know the difference, then they can read the javadoc. The word `stored` doesn't really convey the difference, the user still has to read the javadoc.
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984516641
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the configured System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the configured properties in zookeeper
+ * and not properties from accumulo.properties or defaults.
+ *
+ * @return a map of system properties set in zookeeper
+ */
+ Map<String,String> getSystemProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
> query what properties were set vs what are defaults so the user can know what's been changed, etc.
I'm confused. If the new `modifyProperties()` does not throw an exception, then won't all the properties passed in be set? From a user perspective, is there a situation where the properties I pass to the method don't all get set?
--
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 a diff in pull request #2986: Re-add getProperties() to InstanceOperations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2986:
URL: https://github.com/apache/accumulo/pull/2986#discussion_r984518384
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -93,6 +93,15 @@ void modifyProperties(Consumer<Map<String,String>> mapMutator) throws AccumuloEx
*/
Map<String,String> getSystemConfiguration() throws AccumuloException, AccumuloSecurityException;
+ /**
+ * Retrieves the stored System properties from zookeeper. This method is different from
+ * {@link #getSystemConfiguration()} as it will only return the stored properties in zookeeper and
+ * not properties from accumulo.properties or default values..
+ *
+ * @return a map of stored system properties set in zookeeper
+ */
+ Map<String,String> getStoredProperties() throws AccumuloException, AccumuloSecurityException;
Review Comment:
What about `getOverridenProperties`? This would coincide with the "overridden" label displayed by the Shell config command.
--
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