You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/04/03 16:08:06 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3272: Resolves server to use for scan in a single place
keith-turner opened a new pull request, #3272:
URL: https://github.com/apache/accumulo/pull/3272
The code for resolving which tserver or sserver to use for a scan was spread out across multiple methods responsible for executing a scan. Pulled the code to resolve which server to use into a single place in the code that executes a scan.
Also introduced a new class to represent the server and server type (sserver or tserver) used to process a scan.
These changes clean up two problems in the code. First the tablet server location class was being used to represent a scan server with a special string placed in the tserver session field. Second the decision to use a scan server was deeper in the scan code than error reporting code and the resulted in the need for an odd instance variable to remember that a scan server was used for error reporting. Removing these two problems makes the code easier to modify and maintain.
--
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 #3272: Resolves server to use for scan in a single place
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#issuecomment-1498115816
> > I'm not even sure they should go in 3.0... maybe 3.1 right after we release 3.0.
>
> I want to get these in sooner rather than later so I can build on them. If they do not go into main now, then they could go in the elasticity branch. I was thinking putting them in main may help ease merge conflicts from main to elasticity.
I'm not really opposed to including it in 3.0. I was just expressing a bit of uncertainty, based on the size and not having a lot of understanding of the changes (yet). Based on the description of what it does, it seems reasonable to include sooner. Without having a lot of time to review in depth at the moment, I'm okay with either.
--
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] keith-turner commented on pull request #3272: Resolves server to use for scan in a single place
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#issuecomment-1494610365
This PR pulls improvements out of #3143 into a standalone commit like #3271 did. Doing this because I will need to make different caching changes than #3143 did inorder to support on demand tables and the general improvements made in #3143 will support those changes.
--
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] keith-turner commented on a diff in pull request #3272: Resolves server to use for scan in a single place
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#discussion_r1158952357
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -171,6 +173,44 @@ public static boolean getBatchFromServer(ClientContext context, Range range, Key
throw new AccumuloException("getBatchFromServer: failed");
}
+ enum ServerType {
+ TSERVER, SSERVER
+ }
+
+ static class ScanAddress {
+ final String serverAddress;
+ final ServerType serverType;
+ final TabletLocation tabletInfo;
+
+ public ScanAddress(String serverAddress, ServerType serverType, TabletLocation tabletInfo) {
+ this.serverAddress = Objects.requireNonNull(serverAddress);
+ this.serverType = Objects.requireNonNull(serverType);
+ this.tabletInfo = Objects.requireNonNull(tabletInfo);
+ }
+
+ public KeyExtent getExtent() {
+ return tabletInfo.getExtent();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ ScanAddress that = (ScanAddress) o;
+ return serverAddress.equals(that.serverAddress) && serverType == that.serverType
+ && getExtent().equals(that.getExtent());
+ }
+
+ @Override
+ public int hashCode() {
+ throw new UnsupportedOperationException();
Review Comment:
Can do that. I checked to ensure TabletLocation implements the hashCode function and it does.
--
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 #3272: Resolves server to use for scan in a single place
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#issuecomment-1497571642
Should this start with the 2.1 branch?
--
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 a diff in pull request #3272: Resolves server to use for scan in a single place
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#discussion_r1158809902
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -171,6 +173,44 @@ public static boolean getBatchFromServer(ClientContext context, Range range, Key
throw new AccumuloException("getBatchFromServer: failed");
}
+ enum ServerType {
+ TSERVER, SSERVER
+ }
+
+ static class ScanAddress {
+ final String serverAddress;
+ final ServerType serverType;
+ final TabletLocation tabletInfo;
+
+ public ScanAddress(String serverAddress, ServerType serverType, TabletLocation tabletInfo) {
+ this.serverAddress = Objects.requireNonNull(serverAddress);
+ this.serverType = Objects.requireNonNull(serverType);
+ this.tabletInfo = Objects.requireNonNull(tabletInfo);
+ }
+
+ public KeyExtent getExtent() {
+ return tabletInfo.getExtent();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ ScanAddress that = (ScanAddress) o;
+ return serverAddress.equals(that.serverAddress) && serverType == that.serverType
+ && getExtent().equals(that.getExtent());
+ }
+
+ @Override
+ public int hashCode() {
+ throw new UnsupportedOperationException();
Review Comment:
This seems like an unnecessary restriction. Could instead use:
```suggestion
return Objects.hash(serverAddress, serverType, tabletInfo);
```
--
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] keith-turner commented on pull request #3272: Resolves server to use for scan in a single place
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#issuecomment-1497626850
> Should this start with the 2.1 branch?
I was wondering about that, its only a reorganization of the code. The pros I can think of for going to 2.1 are :
* Makes it easier to maintain 2.1
* Makes it easier to merge from 2.1 to 3
The only con I can think of is that these changes may introduce a bug in 2.1.
Are there any other pros or cons?
If this change were pulled back to 2.1 then would also want to pull #3273 back. There is also another simple change I was thinking of making in main. Wanted to rename the TabletLocator to TabletCache to reflect the changes that will be made in the elasticity branch to generalize the responsibility of the client side tablet cache. The reason I am making all of these changes in main is to make merging to the elasticity branch easier. I plan to make the actual changes to cache functionality to support ondemand tablets only in the elasticity branch. All of these prereq changes for that cahce changes will have been made in main. So would it make sense to make all of these prereq changes in 2.1 instead?
--
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 #3272: Resolves server to use for scan in a single place
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#issuecomment-1497548354
This looks good to me. I kicked off a full IT build, I'm pretty sure that we have ITs that test the tserver fallback case where a scan server is not found and good coverage in the other ITs too.
--
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] keith-turner commented on pull request #3272: Resolves server to use for scan in a single place
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#issuecomment-1498129771
> This looks good to me. I kicked off a full IT build, I'm pretty sure that we have ITs that test the tserver fallback case where a scan server is not found and good coverage in the other ITs too.
OK I switched this to target the elasticity branch. I think its ok to merge there w/o a full IT, so I am going to merge this. Let me know if something does blow up the with the ITs though.
--
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] keith-turner commented on pull request #3272: Resolves server to use for scan in a single place
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#issuecomment-1497979145
> I'm not even sure they should go in 3.0... maybe 3.1 right after we release 3.0.
I want to get these in sooner rather than later so I can build on them. If they do not go into main now, then they could go in the elasticity branch. I was thinking putting them in main may help ease merge conflicts from main to elasticity.
--
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] keith-turner merged pull request #3272: Resolves server to use for scan in a single place
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3272:
URL: https://github.com/apache/accumulo/pull/3272
--
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 #3272: Resolves server to use for scan in a single place
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3272:
URL: https://github.com/apache/accumulo/pull/3272#issuecomment-1497853201
Given the quantity of the changes, I don't think it makes sense to make these prereq changes in 2.1. I'm not even sure they should go in 3.0... maybe 3.1 right after we release 3.0.
--
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