keith-turner commented on code in PR #5072:
URL: https://github.com/apache/accumulo/pull/5072#discussion_r1847008790
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -312,6 +312,10 @@ List<ActiveCompaction> getActiveCompactions(ServerId
server)
*/
List<ActiveCompaction> getActiveCompactions() throws AccumuloException,
AccumuloSecurityException;
+ List<ActiveCompaction> getActiveCompactions(Predicate<String>
resourceGroupPredicate,
Review Comment:
Looking at this PR I am seeing some preexisting problems. This comment is
about trying to figure out a good goal state.
One problem I noticed is that listcompactoins uses multiple threads to
gather information and list scans does not. Looking into this the first
question I had was if list scans were to be made multi-threaded, then where
should the actual multi-threading happen? Should it happen in shell code or in
api impl code. Another problem I noticed is that this new method seemed to
somewhat overlap with some other instance operations methods like
`List<ActiveCompaction> getActiveCompactions(ServerId server)`, `Set<ServerId>
getServers(ServerId.Type type, Predicate<String> resourceGroupPredicate,
BiPredicate<String,Integer> hostPortPredicate)`, and `List<ActiveCompaction>
getActiveCompactions()`. So this lead to a second question of how can we
minimize the InstanceOperations API?
One possible way to minimize the APIs and consistently support
multi-threading (doing so in the client impl) is with the following API changes
to InstanceOperations.
* remove `List<ActiveScan> getActiveScans(ServerId server)` this is new in
4.0.0, so no deprecation needed can just remove it.
* remove `List<ActiveCompaction> getActiveCompactions(ServerId server)`
this is also new in 4.0.0
* remove ` List<ActiveCompaction> getActiveCompactions(Predicate<String>
resourceGroupPredicate, BiPredicate<String,Integer> hostPortPredicate)` method
added in this PR
* maybe deprecate `List<ActiveCompaction> getActiveCompactions()`, this
does not have to be done would only be done to minimize the API
* add method `List<ActiveScan> getActiveScans(Collections<ServerId>
servers)` that will gather active scans from servers using multiple threads
similar to what getActive compactions does.
* add method `List<ActiveCompaction>
getActiveCompactions(Collection<ServerId> server)` that will gather active
compactions from servers using multiple threads
With the above changes the existing methods to get servers can be used to
get the collection of servers ids then there is a single get scans or get
compactions methods for gathering information for those servers.
If that overall goal makes sense then in this PR maybe we could add and use
the `List<ActiveCompaction> getActiveCompactions(Collection<ServerId> server)`
method in this PR and open follow on issue to clean up InstanceOperations and
make list scans multithreaded.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ListScansCommand.java:
##########
@@ -75,14 +77,31 @@ public int numArgs() {
public Options getOptions() {
final Options opts = new Options();
- tserverOption = new Option("ts", "tabletServer", true, "tablet server to
list scans for");
- tserverOption.setArgName("tablet server");
- opts.addOption(tserverOption);
+ serverOpt = new Option("s", "server", true, "tablet/scan server regex to
list scans for");
+ serverOpt.setArgName("tablet/scan server regex");
+ opts.addOption(serverOpt);
+
+ rgOpt = new Option("rg", "resourceGroup", true,
+ "tablet/scan server resource group regex to list scans for");
+ rgOpt.setArgName("resource group");
+ opts.addOption(rgOpt);
disablePaginationOpt = new Option("np", "no-pagination", false, "disable
pagination of output");
opts.addOption(disablePaginationOpt);
return opts;
}
+ static BiPredicate<String,Integer> serverRegexPredicate(CommandLine cl,
Option serverOpt) {
+ return Optional.ofNullable(cl.getOptionValue(serverOpt))
+ .map(regex -> (BiPredicate<String,
+ Integer>) (h, p) -> Pattern.compile(regex).matcher(h + ":" +
p).matches())
+ .orElse((h, p) -> true);
Review Comment:
This seemed like its compiling the regex for each call to the predicate.
Was wondering if this could be avoided experimented locally and came up with
the following. I did not know about `.asMatchPredicate()` until seeing it a
few lines down, that is really neat.
```suggestion
return
Optional.ofNullable(cl.getOptionValue(serverOpt)).map(regex->Pattern.compile(regex).asMatchPredicate())
.map(matcherPredicate -> (BiPredicate<String, Integer>) (h, p)
-> matcherPredicate.test(h+":"+p)).orElse((h, p) -> true);
```
--
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]
