paul-rogers commented on issue #1910: DRILL-7393: Revisit Drill tests to ensure that patching is executed b… URL: https://github.com/apache/drill/pull/1910#issuecomment-560539390 @vvysotskyi, thanks for the additional info. Doing the patching in a static block makes sense; that ensures the patching is done early (though it still depends on when the class is loaded.) Putting the patching in a test base class solves the ordering issue: the static block will be invoked before any other code in the test. However, the problem with a base class is we'd need a common base class for all tests, as was done in this PR. This means that we then mask the Drillbit patching, which, as explained earlier, we don't want to do. I wonder, can we define an interface to solve the problem? Something like this: ``` public interface RequiresGuavaPatch { GuavaPatcher PATCHER = new GuavaPatcher(); } public class MyTest implements RequiresGuavaPatch { ... } ``` Assuming that creating the patcher does the patches. Else, we can create a specialized "shim" class that does patching in its constructor. This approach hides the details of patching, yet declares that the test does require patching. Only non-Drillbit tests need worry about this interface. And, only tests that have affected dependencies need implement the interface. An alternative is to be more specific, `HBaseTest`, `MapRTest` and so on that do whatever patching is needed just for that dependency. As these dependencies upgrade their own dependencies, we can change the code behind, say, `MapRTest` to remove patching no longer needed. This way, we track patching at the dependency level, rather than as a single global (and thus hard-to-understand) task. Let me clarify that point a bit. We need to patch due to specific transitive dependencies. As those are removed, we should remove patches. If we know that such-and-so patching is needed for each dependency, we can track when that patching is no longer needed. If patching is just done globally, then we have no good way to know why we do each patch, and when it is no longer needed. Would something like this solve the problem?
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services
