Resolves server to use for scan in a single place #3272
Conversation
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 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. |
|
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 :
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? |
|
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. |
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java
Outdated
Show resolved
Hide resolved
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. |
…canner.java Co-authored-by: Christopher Tubbs <[email protected]>
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. |
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. |
4 participants
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.