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/10/23 20:24:34 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1752: New unload thrift endpoint. Closes #1750
milleruntime opened a new pull request #1752:
URL: https://github.com/apache/accumulo/pull/1752
* Create new thrift endpoint called unload to replace unloadTablet that
uses a string type for a param instead of the enum TUnloadTabletGoal
* This will allow us to drop the unused thrift enum TUnloadTabletGoal
and clean up code in Master to just pass a string for the goal state
since that is all that is needed in UnloadTabletHandler
My hope is that no matter what clean up can be done in Master, passing a string to the tserver to perform the unload will suffice.
----------------------------------------------------------------
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 #1752: New unload thrift endpoint. Closes #1750
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1752:
URL: https://github.com/apache/accumulo/pull/1752#discussion_r511171869
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1454,9 +1454,16 @@ public void run() {
}
}
+ @Deprecated
@Override
public void unloadTablet(TInfo tinfo, TCredentials credentials, String lock, TKeyExtent textent,
TUnloadTabletGoal goal, long requestTime) {
+
Review comment:
For RPC compatibility for rolling restarts, you should probably have a real implementation here, by calling the new unload method with the toString version of the goal.
##########
File path: core/src/main/thrift/tabletserver.thrift
##########
@@ -348,6 +350,16 @@ service TabletClientService extends client.ClientService {
7:i64 requestTime
)
+ // since 2.1
+ oneway void unload(
+ 5:trace.TInfo tinfo
+ 1:security.TCredentials credentials
+ 4:string lock
+ 2:data.TKeyExtent extent
+ 6:string goal
+ 7:i64 requestTime
Review comment:
These fields could be numbered sanely, if you're creating a new function. The old fields were numbered that way because it evolved over time.
----------------------------------------------------------------
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] milleruntime commented on pull request #1752: New unload thrift endpoint. Closes #1750
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1752:
URL: https://github.com/apache/accumulo/pull/1752#issuecomment-716658357
> It looks like the only action of this PR is to replace an enum type with a String type. I don't see the advantage of that. Concrete types gives us safety and prevent bugs. It is normal for us to have Thrift-type and a non-Thrift-type that we have to convert between, in order to avoid leaking Thrift types into our code. I'm not sure it's better to replace concrete types with String to avoid that redundancy.
The motivation for this change was to help with cleaning up all the enums in Master code. I wasn't thrilled about this change, when I saw all that was required to remove the thrift enum. I will close this for now and try to clean up the code without having to change the Thrift endpoint.
----------------------------------------------------------------
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] milleruntime closed pull request #1752: New unload thrift endpoint. Closes #1750
Posted by GitBox <gi...@apache.org>.
milleruntime closed pull request #1752:
URL: https://github.com/apache/accumulo/pull/1752
----------------------------------------------------------------
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