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/01/12 16:53:05 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2415: Create new external compactions admin utility
milleruntime opened a new pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415
TODO Figure out Thrift RPC calls for cancelling. I was thinking of adding a thrift method to the `CompactionCoordinator` called `cancel()` that will call cancel on the appropriate compactor. I think this would probably require a second cancel thrift method on `CompactorService`.
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r786842667
##########
File path: server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java
##########
@@ -360,6 +360,12 @@ private void cancel(String externalCompactionId) throws TException {
}
}
+ @Override
+ public void cancel(TInfo tinfo, TCredentials credentials, String externalCompactionId)
+ throws TException {
Review comment:
oh ok. I'm not up to speed on all of the permissions, I just know I used it on all of the other thrift api methods. If it's not appropriate, then that is good with me.
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r786958170
##########
File path: server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java
##########
@@ -360,6 +360,12 @@ private void cancel(String externalCompactionId) throws TException {
}
}
+ @Override
+ public void cancel(TInfo tinfo, TCredentials credentials, String externalCompactionId)
+ throws TException {
Review comment:
I saw that the FATE Table cancel compact operation has the same permissions as a user needs to compact. So we could try that:
From FateServiceHandler:
` canCancelCompact = manager.security.canCompact(c, tableId, namespaceId);
`
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r788033023
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
##########
@@ -130,6 +130,7 @@ public void execute(final String[] args) {
private void cancelCompaction(ServerContext context, String ecid) {
CompactionCoordinatorService.Client coordinatorClient = null;
+ ecid = ExternalCompactionUtil.prefixECID(ecid);
Review comment:
`ExternalCompactionId.of` also validates that the id is a UUID. If we are going to have another method to take a String and massage it into an ExternalCompactionId, then maybe we can move your method body into `ExternalCompactionId.from(String)` or something like that. Then at least we can re-use PREFIX.
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r789135726
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/ExternalCompactionId.java
##########
@@ -56,4 +56,14 @@ public static ExternalCompactionId of(String id) {
return new ExternalCompactionId(id);
}
+ /**
+ * Sanitize user input for the ECID string with proper "ECID:" prefix.
+ */
+ public static ExternalCompactionId from(String ecid) {
+ ecid = ecid.replace("ecid:", "ECID:");
Review comment:
Whoops, good catch.
--
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 #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#issuecomment-1013431932
I think this PR is ready for review. This creates the admin tool for external compactions but only with useful commands that currently work. I added RPCs for cancelling but have some ideas for a follow on task to provide more command and control.
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r786715603
##########
File path: server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java
##########
@@ -360,6 +360,12 @@ private void cancel(String externalCompactionId) throws TException {
}
}
+ @Override
+ public void cancel(TInfo tinfo, TCredentials credentials, String externalCompactionId)
+ throws TException {
Review comment:
The other thrift api methods perform a security check first, do we want to do that 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] milleruntime commented on pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#issuecomment-1011307026
> > I was thinking of adding a thrift method to the `CompactionCoordinator` called `cancel()` that will call cancel on the appropriate compactor.
>
> Would this address #2386?
Yup, that is the plan. I couldn't find a good way for the user to cancel the compaction so I made this external compactions admin utility.
--
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 pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#issuecomment-1015377056
This looks correct. Just one comment about API security check.
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r789132825
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/schema/ExternalCompactionId.java
##########
@@ -56,4 +56,14 @@ public static ExternalCompactionId of(String id) {
return new ExternalCompactionId(id);
}
+ /**
+ * Sanitize user input for the ECID string with proper "ECID:" prefix.
+ */
+ public static ExternalCompactionId from(String ecid) {
+ ecid = ecid.replace("ecid:", "ECID:");
Review comment:
You can use PREFIX 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.
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 pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#issuecomment-1018467248
Changes LGTM.
--
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 #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#issuecomment-1016709740
FYI @dlmarion I made some improvements in 4c78355 based on your comments. I just wanted to give you a heads up since you already approved this PR.
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r788029804
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
##########
@@ -130,6 +130,7 @@ public void execute(final String[] args) {
private void cancelCompaction(ServerContext context, String ecid) {
CompactionCoordinatorService.Client coordinatorClient = null;
+ ecid = ExternalCompactionUtil.prefixECID(ecid);
Review comment:
> I assume you are trying to clean up bad user input here?
Yes.
> Can we just use ExternalCompactionId.of(String) here and let the user pass the right thing? Maybe we can clean up the error messages in that method to be more specific about the issues.
I wasn't sure. I did this in the monitor just because it wasn't clear to the user that the server code required the prefix and I didn't like relying on trial and error to figure that out. Even with a nice error message, I don't like users having to guess if you need the prefix or not.
I pinged @keith-turner on chat asking if he had any thoughts behind the origin of the prefix but haven't heard back yet.
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r786833747
##########
File path: server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java
##########
@@ -360,6 +360,12 @@ private void cancel(String externalCompactionId) throws TException {
}
}
+ @Override
+ public void cancel(TInfo tinfo, TCredentials credentials, String externalCompactionId)
+ throws TException {
Review comment:
I didn't think it needed SYSTEM level permission (root) like the other method required. I thought it should require at least some level of USER permission but wasn't sure since the permissions are so confusing. Any ideas?
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r788006437
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
##########
@@ -130,6 +130,7 @@ public void execute(final String[] args) {
private void cancelCompaction(ServerContext context, String ecid) {
CompactionCoordinatorService.Client coordinatorClient = null;
+ ecid = ExternalCompactionUtil.prefixECID(ecid);
Review comment:
I assume you are trying to clean up bad user input here? Can we just use `ExternalCompactionId.of(String)` here and let the user pass the right thing? Maybe we can clean up the error messages in that method to be more specific about the issues.
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r788048144
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
##########
@@ -130,6 +130,7 @@ public void execute(final String[] args) {
private void cancelCompaction(ServerContext context, String ecid) {
CompactionCoordinatorService.Client coordinatorClient = null;
+ ecid = ExternalCompactionUtil.prefixECID(ecid);
Review comment:
I like the idea of consolidating the logic into `ExternalCompactionId`. The only issue is that both places I need a String and not the object. What about making the prefix method static member of `ExternalCompactionId`? Then we could reuse the validation in the `of()` 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] dlmarion commented on a change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r788079834
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
##########
@@ -130,6 +130,7 @@ public void execute(final String[] args) {
private void cancelCompaction(ServerContext context, String ecid) {
CompactionCoordinatorService.Client coordinatorClient = null;
+ ecid = ExternalCompactionUtil.prefixECID(ecid);
Review comment:
Right, but on the server side, it parses the incoming ecid string into an ExternalCompactionId. It would be good to know that on the client side it's a valid ExternalCompactionId before sending it. You can always get the String value from the ExternalCompactionId, right? To be clear, I'm suggesting something like `ExternalCompactionId.from(String).toString()`. Whether you call the method `from`,`parse`, etc. doesn't matter to me. You called the method `prefix`, but it looks like it's just trying to create a valid ECID from the user input.
--
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 change in pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#discussion_r789128632
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
##########
@@ -130,6 +130,7 @@ public void execute(final String[] args) {
private void cancelCompaction(ServerContext context, String ecid) {
CompactionCoordinatorService.Client coordinatorClient = null;
+ ecid = ExternalCompactionUtil.prefixECID(ecid);
Review comment:
Created `from` method in b233afe
--
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] DomGarguilo commented on pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415#issuecomment-1011282688
> I was thinking of adding a thrift method to the `CompactionCoordinator` called `cancel()` that will call cancel on the appropriate compactor.
Would this address #2386?
--
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 merged pull request #2415: Create new external compactions admin utility
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #2415:
URL: https://github.com/apache/accumulo/pull/2415
--
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