> On Jan. 22, 2019, 11:45 p.m., Slim Bouguerra wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/AsyncTaskCopyAuxJars.java > > Lines 83 (patched) > > <https://reviews.apache.org/r/69808/diff/1/?file=2121335#file2121335line83> > > > > Are you tizing the compiler ? i think you can inline that string.
This code was copied from the old monolith LlapServiceDriver as it was. All over the code error logging is done in parallel to the logger and also to System.err. I totally agree, that it is not ok how it is done, and actually my plan for the next refactoring of this code is to fix the logging. But the scope of this modification is to cut it to pieces, and didn't wanted to do too much modification on the actual codes. > On Jan. 22, 2019, 11:45 p.m., Slim Bouguerra wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/AsyncTaskCopyAuxJars.java > > Lines 84 (patched) > > <https://reviews.apache.org/r/69808/diff/1/?file=2121335#file2121335line84> > > > > please do not print to System channel. Logger is enough This code was copied from the old monolith LlapServiceDriver as it was. All over the code error logging is done in parallel to the logger and also to System.err. I totally agree, that it is not ok how it is done, and actually my plan for the next refactoring of this code is to fix the logging. But the scope of this modification is to cut it to pieces, and didn't wanted to do too much modification on the actual codes. > On Jan. 22, 2019, 11:45 p.m., Slim Bouguerra wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/AsyncTaskCopyAuxJars.java > > Lines 89 (patched) > > <https://reviews.apache.org/r/69808/diff/1/?file=2121335#file2121335line89> > > > > final field ? At least 80% of the local variables could be declared as final, which mainly only increases the volume of the code. I usually only declare them so if they are member variables, or need to be declared final if in order to be able to access it. The purpose of this refactoring is to cut the code into smaller pieces where each function can be fit into one screen, each variables scope is only a few lines, so it can be seen, how it is used. > On Jan. 22, 2019, 11:45 p.m., Slim Bouguerra wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/AsyncTaskCopyAuxJars.java > > Lines 130 (patched) > > <https://reviews.apache.org/r/69808/diff/1/?file=2121335#file2121335line130> > > > > do you mean if(hasException) ? > > not sure am getting the logic The logic here is to handle the situation when the jarPath could not be identified, but not due to an exception, mainly because it can not be located. The aim of this refactoring is to move the pieces of this monolith class into classes that do a smaller logical unit. I agree with you, that this part is a bit messy, but I didn't wanted to modify the existing codes too much, just to cut them to pieces now. Maybe during a later refactoring. - Miklos ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69808/#review212207 ----------------------------------------------------------- On Jan. 22, 2019, 11:22 p.m., Miklos Gergely wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69808/ > ----------------------------------------------------------- > > (Updated Jan. 22, 2019, 11:22 p.m.) > > > Review request for hive and Ashutosh Chauhan. > > > Bugs: HIVE-21149 > https://issues.apache.org/jira/browse/HIVE-21149 > > > Repository: hive-git > > > Description > ------- > > LlapServiceDriver is one monolith class doing several things, needs to be > refactor in order to make it clearer how it works. > > > Diffs > ----- > > bin/ext/llap.sh 91a54b3 > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/LlapOptionsProcessor.java > 2445075 > llap-server/src/java/org/apache/hadoop/hive/llap/cli/LlapServiceDriver.java > ffdd340 > llap-server/src/java/org/apache/hadoop/hive/llap/cli/LlapSliderUtils.java > bdec1c1 > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/AsyncTaskCopyAuxJars.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/AsyncTaskCopyConfigs.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/AsyncTaskCopyLocalJars.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/AsyncTaskCreateUdfFile.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/AsyncTaskDownloadTezJars.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/LlapConfigJsonCreator.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/LlapServiceCommandLine.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/LlapServiceDriver.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/LlapTarComponentGatherer.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/cli/service/package-info.java > PRE-CREATION > > llap-server/src/test/org/apache/hadoop/hive/llap/cli/service/TestLlapServiceCommandLine.java > PRE-CREATION > > llap-server/src/test/org/apache/hadoop/hive/llap/cli/service/package-info.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/69808/diff/1/ > > > Testing > ------- > > Tested on actual cluster, llap still starts up fine. > > > Thanks, > > Miklos Gergely > >
