HIVE-16866 : existing available UDF is used in TestReplicationScenariosAcrossInstances#testDropFunctionIncrementalReplication #193
Conversation
| */ | ||
| static public FileStatus getFileStatus(Path src, String chksumString, |
There was a problem hiding this comment.
Why public clause is removed for some methods and kept at some?
There was a problem hiding this comment.
the idea is to give most restrictive scope visibility for methods / varaibles/ classes. Depending on what where its accessed for now the visibility has been changed
| + ".testFunction as 'com.yahoo.sketches.hive.theta.DataToSketchUDAF' " | ||
| + "using jar 'ivy://com.yahoo.datasketches:sketches-hive:0.8.2'"); | ||
| + ".testFunction as 'hivemall.tools.string.StopwordUDF' " | ||
| + "using jar 'ivy://io.github.myui:hivemall:0.4.0-2'"); | ||
| WarehouseInstance.Tuple bootStrapDump = primary.dump(primaryDbName, null); | ||
| replica.load(replicatedDbName, bootStrapDump.dumpLocation) | ||
| .run("REPL STATUS " + replicatedDbName) |
There was a problem hiding this comment.
After bootstrap Repl load, need to check if the function was properly loaded or not. Or else, after drop we cannot ensure if function is missing from beginning or really DROP has been applied.
There was a problem hiding this comment.
I am verifying that the replica database has the correct last repl id. The fact that the event was missed with the last repl id changed is the use case you are mentioning i think, For this there are incremental replication tests which specifically checks for the existence of the function and i dont think we need it here.
There was a problem hiding this comment.
My query was, if CREATE FUNCTION fails due to jar unaccessible or any other reasons, then bootstrap won't have it and so lastReplId verification alone won't be enough to ensure that. Apparently, another test testCreateFunctionIncrementalReplication uses the same jar and verifies it. That's fine.
| */ | ||
| static Path getCMPath(Configuration conf, String checkSum) | ||
| throws IOException, MetaException { | ||
| static Path getCMPath(Configuration conf, String checkSum) throws IOException, MetaException { |
There was a problem hiding this comment.
Looks like getCMPath never throw IOException or MetaException. Shall check and remove it.
There was a problem hiding this comment.
I did not change the method signature for now. just reformatted the code
There was a problem hiding this comment.
Just noticed that those exceptions are not thrown by this method and as we making changes in this code, thought may be make these changes as well. If you don't want to make those changes part of this path, that's fine as well.
There was a problem hiding this comment.
@sankarh unfortunately i was not confident about the change, as I had just formatted the code and requires very less diligence on my side, but i have since then, looked back in it and made the requested modifications from you. I am waiting for apache jira to come back to attach the new patch
There was a problem hiding this comment.
@anishek Thejas already committed your patch to apache/master. We'll accommodate this change later with some other patch. :)
…osAcrossInstances#testDropFunctionIncrementalReplication
There are few javadoc changes and a method rename in repl change manager along with a few access modifiers as well.