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 2020/09/06 19:57:15 UTC
[GitHub] [accumulo] jkosh44 opened a new pull request #1701: Add method to get properties with a prefix
jkosh44 opened a new pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701
Closes #1627
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on a change in pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#discussion_r484111856
##########
File path: core/src/main/java/org/apache/accumulo/core/client/PluginEnvironment.java
##########
@@ -55,6 +55,15 @@
*/
String get(String key);
+ /**
+ * Returns all properties with a given prefix
+ *
+ * @param prefix
+ * prefix of properties to be returned
+ * @return all properties with a given prefix
+ */
Review comment:
Thanks, added
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-687980505
@ctubbsii Sorry about the force pushing, bad habits. I'll add the verify to my tests, thanks for the info, I'm not as familiar with EasyMock as I'd like to be.
Did you see my comment about the prefix subset?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-687917091
> One open question is should I be filtering out sensitive properties?
This is for pluggable components, vetted by an admin, and placed on a server's class path. So, it has access to everything anyway. I don't think there'd be any point.
Also, did you see the test failure? I'm not sure if that's related to these changes, but might be worth looking into.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-687888020
One open question is should I be filtering out sensitive properties?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-687999540
Thanks for the PR @jkosh44 !
I see you've contributed before, but you're not on our "people" page, listing contributors. If you wish to be added as a contributor to https://accumulo.apache.org/people/ , please open a pull request to add yourself at https://github.com/apache/accumulo-website/edit/master/pages/people.md and leave a reference to `apache/accumulo#1701` in your commit log.
If you intend to be a regular contributor to Accumulo projects, please consider subscribing to our developer mailing list (https://accumulo.apache.org/contact-us/) and introducing yourself. :smiley_cat:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on a change in pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#discussion_r484108048
##########
File path: core/src/main/java/org/apache/accumulo/core/client/PluginEnvironment.java
##########
@@ -55,6 +55,15 @@
*/
String get(String key);
+ /**
+ * Returns all properties with a given prefix
+ *
+ * @param prefix
+ * prefix of properties to be returned
+ * @return all properties with a given prefix
+ */
Review comment:
I know this should probably have a `@since` tag, I'm just not sure what version to put. Please let me know and I'll add 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on a change in pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#discussion_r484121839
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServiceEnvironmentImpl.java
##########
@@ -78,6 +79,22 @@ public String get(String key) {
}
}
+ @Override
+ public Map<String,String> getWithPrefix(String prefix) {
+ if (Property.isValidPropertyPrefix(prefix)) {
+ Property propertyPrefix = Property.getPropertyByKey(prefix);
Review comment:
Fixed
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServiceEnvironmentImpl.java
##########
@@ -78,6 +79,22 @@ public String get(String key) {
}
}
+ @Override
+ public Map<String,String> getWithPrefix(String prefix) {
+ if (Property.isValidPropertyPrefix(prefix)) {
+ Property propertyPrefix = Property.getPropertyByKey(prefix);
+ return acfg.getAllPropertiesWithPrefix(propertyPrefix);
+ } else {
+ Map<String,String> properties = new HashMap<>();
+ for (Entry<String,String> prop : acfg) {
+ if (prop.getKey().startsWith(prefix)) {
+ properties.put(prop.getKey(), prop.getValue());
+ }
+ }
+ return properties;
+ }
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-687987707
I agree with the complexity comment. I'm good to merge if you are.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#discussion_r484151738
##########
File path: server/base/src/test/java/org/apache/accumulo/server/ServiceEnvironmentImplTest.java
##########
@@ -44,6 +46,12 @@ public void setUp() {
serviceEnvironment = new ServiceEnvironmentImpl(srvCtx);
}
+ @After
+ public void verifyMocks() {
+ verify(srvCtx);
+ verify(acfg);
+ }
+
Review comment:
The verify method is actually a varargs, so you can pass both in one:
```suggestion
@After
public void verifyMocks() {
verify(srvCtx, acfg);
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-687982527
> Did you see my comment about the prefix subset?
I missed it the first time. I think I see what you're suggesting, but I don't think it's worth it. If you can show that it's a substantive performance advantage, then it might make sense... but I suspect it'd be a negligible difference, and just more code complexity to maintain, without real benefit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-687971050
> I did not see the test failure, did it happen during the checks? If so can you send a link? I thought all the checks passed.
https://github.com/apache/accumulo/runs/1078818587
It happened with commit f4e3c51efed22f9c17185bf023883ecb1962e6e9 . It might just have been a transient issue with GitHub Actions containers, or an unrelated problem. It did not seem to reoccur with your latest commits.
Speaking of your commits. You don't need to force-push when update the PR. In fact, it actually makes it harder to review, because it GitHub sends notifications to followers showing the diff between the current code and the previous... but that link doesn't work on force-pushes, because the earlier commits are no longer present. When it comes time to merge, we'll squash it all down to one, anyway, so it's better to just add commits instead of force-pushing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on a change in pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#discussion_r484121833
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -1233,6 +1233,18 @@ public static boolean isValidPropertyKey(String key) {
return validProperties.contains(key) || isKeyValidlyPrefixed(key);
}
+ /**
+ * Checks if the given property prefix is valid. A valid property prefix is equal to some prefix
+ * defined in this class.
+ *
+ * @param prefix
+ * property prefix
+ * @return true if prefix is valid (recognized)
+ */
+ public static boolean isValidPropertyPrefix(String prefix) {
+ return validPrefixes.contains(prefix);
+ }
+
Review comment:
Removed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#discussion_r484119541
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServiceEnvironmentImpl.java
##########
@@ -78,6 +79,22 @@ public String get(String key) {
}
}
+ @Override
+ public Map<String,String> getWithPrefix(String prefix) {
+ if (Property.isValidPropertyPrefix(prefix)) {
+ Property propertyPrefix = Property.getPropertyByKey(prefix);
+ return acfg.getAllPropertiesWithPrefix(propertyPrefix);
+ } else {
+ Map<String,String> properties = new HashMap<>();
+ for (Entry<String,String> prop : acfg) {
+ if (prop.getKey().startsWith(prefix)) {
+ properties.put(prop.getKey(), prop.getValue());
+ }
+ }
+ return properties;
+ }
Review comment:
This can be more succinctly written with streams:
```suggestion
return StreamSupport.stream(acfg.spliterator(), false)
.filter(prop -> prop.getKey().startsWith(prefix))
.collect(Collectors.toMap(Entry::getKey, Entry::getValue));
```
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServiceEnvironmentImpl.java
##########
@@ -78,6 +79,22 @@ public String get(String key) {
}
}
+ @Override
+ public Map<String,String> getWithPrefix(String prefix) {
+ if (Property.isValidPropertyPrefix(prefix)) {
+ Property propertyPrefix = Property.getPropertyByKey(prefix);
Review comment:
This check for valid prefix property can be avoided, by checking the returned object from `Property.getPropertyByKey(prefix)`:
```suggestion
Property propertyPrefix = Property.getPropertyByKey(prefix);
if (propertyPrefix != null && propertyPrefix.getType() == PropertyType.PREFIX) {
```
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -1233,6 +1233,18 @@ public static boolean isValidPropertyKey(String key) {
return validProperties.contains(key) || isKeyValidlyPrefixed(key);
}
+ /**
+ * Checks if the given property prefix is valid. A valid property prefix is equal to some prefix
+ * defined in this class.
+ *
+ * @param prefix
+ * property prefix
+ * @return true if prefix is valid (recognized)
+ */
+ public static boolean isValidPropertyPrefix(String prefix) {
+ return validPrefixes.contains(prefix);
+ }
+
Review comment:
This method isn't necessary, since this loop can be avoided.
```suggestion
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#discussion_r484111074
##########
File path: core/src/main/java/org/apache/accumulo/core/client/PluginEnvironment.java
##########
@@ -55,6 +55,15 @@
*/
String get(String key);
+ /**
+ * Returns all properties with a given prefix
+ *
+ * @param prefix
+ * prefix of properties to be returned
+ * @return all properties with a given prefix
+ */
Review comment:
2.1.0 would be the version we would add this kind of thing to.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-687922386
> > One open question is should I be filtering out sensitive properties?
>
> This is for pluggable components, vetted by an admin, and placed on a server's class path. So, it has access to everything anyway. I don't think there'd be any point.
>
> Also, did you see the test failure? I'm not sure if that's related to these changes, but might be worth looking into.
I did not see the test failure, did it happen during the checks? If so can you send a link? I thought all the checks passed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-688008338
Thanks for the review @ctubbsii!
I'm not sure how regularly I plan on contributing, but I'll do all that now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on a change in pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#discussion_r484152483
##########
File path: server/base/src/test/java/org/apache/accumulo/server/ServiceEnvironmentImplTest.java
##########
@@ -44,6 +46,12 @@ public void setUp() {
serviceEnvironment = new ServiceEnvironmentImpl(srvCtx);
}
+ @After
+ public void verifyMocks() {
+ verify(srvCtx);
+ verify(acfg);
+ }
+
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jkosh44 commented on pull request #1701: Add method to get properties with a prefix
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on pull request #1701:
URL: https://github.com/apache/accumulo/pull/1701#issuecomment-687922256
> Overall, this looks good to me. I made some suggestions for improvements, and I think this could use a unit test, but otherwise, looks okay.
>
> Are you able to include a unit test for this?
@ctubbsii I've made the changes you suggested (and I'm adding a unit test now). One scenario I was thinking about is if the provided prefix is a subset of a recognized prefix.
For example, let's say we are given the prefix "a.b.c" and "a.b" is a recognized prefix. Then it may be more efficient to check for this scenario and only iterate through "a.b"'s properties instead of iterating through all the properties. What do you think (if that made sense)?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org