> On Jan. 27, 2017, 3:37 p.m., Peter Vary wrote:
> > Hi Sergio,
> >
> > Looking through the patch I did not find anything useful to enhance what
> > you did :)
> >
> > I did run the yetus pre-commit stuff on it (author, checkstyle, javadoc,
> > findbugs, whitespace, asflicense), and it came up with the following nits:
> >
> > ./ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:1:/**: warning:
> > File length is 3,809 lines (max allowed is 2,000).
> > ./ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:2098: public
> > static ContentSummary getInputSummary(final Context ctx, MapWork work,
> > PathFilter filter):3: warning: Method length is 179 lines (max allowed is
> > 150).
> > ./ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:2140:
> > new
> > ThreadFactoryBuilder().setDaemon(true).setNameFormat("Get-Input-Summary-%d").build());:
> > warning: Line is longer than 100 characters (found 102).
> > ./ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:2192:
> > if (partDesc.getTableDesc() != null &&
> > partDesc.getTableDesc().getProperties() != null) {: warning: Line is longer
> > than 100 characters (found 105).
> > ./ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:2193:
> > metaTableStorage =
> > partDesc.getTableDesc().getProperties().getProperty(hive_metastoreConstants.META_TABLE_STORAGE,
> > null);: warning: Line is longer than 100 characters (found 139).
> > ./ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:2196:
> > metaTableStorage =
> > partDesc.getProperties().getProperty(hive_metastoreConstants.META_TABLE_STORAGE,
> > metaTableStorage);: warning: Line is longer than 100 characters (found
> > 136).
> >
> > I do not think that we are able to keep files shorter than 2000 lines in
> > Hive, so I will remove that check, but AFAIK we try to keep the line length
> > to 100.
> >
> > Or should I change that too?
> >
> > Thanks,
> > Peter
>
> Sergio Pena wrote:
> Thanks Peter for the details. I don't know how we would fix those kind of
> issues. I've seen too large lines on the Hive code, and fixing them will take
> a while. Turning off that checking would be better.
>
> Peter Vary wrote:
> Hi Sergio,
>
> Just to clarify: These are the new checkstyle missalignments introduced
> by the patch. The other few hundred are left out :). Yetus only reports on
> new stuff.
Ah ok. Why are we trying to keep the line length to 100? With today's new
monitors and laptops display resolutions, 100 is less than half of the display.
And regarding the method length. I didn't add any new line to the method, but
yetus warn us that is too large. I agree on method size should be less, but in
this patch I just edited a few lines only.
Isn't yetus too much verbose on these warnings about line limits?
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55994/#review163277
-----------------------------------------------------------
On Jan. 27, 2017, 7:21 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55994/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 7:21 p.m.)
>
>
> Review request for hive, Mohit Sabharwal, Sahil Takiar, and Vihang
> Karajgaonkar.
>
>
> Bugs: HIVE-15736
> https://issues.apache.org/jira/browse/HIVE-15736
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Added unit tests on TestUtilities to validate
> - Single and multiple threads
> - InputEstimator usage
> - ContentSummaryInputFormat usage.
>
> It also fixed an issue with the InputEstimator scenario where the values
> returned by the InputEstimator where overriden later by the correct
> filesystem calls.
>
> An interesting thing (code commented while it is on review) is that when
> executing the InputEstimator code path the line commented seems are not
> needed. It might
> be that the idea was to set some configurations to the jobConf, but the
> jobConf was never passed as parameter to the estimate method. Please help me
> verify this.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java
> 68dd5e7247415dec1e353010ea34481c4f2fc6cd
> ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java
> b2c586507483eb5588e07a9ceba3caf395b4d607
> ql/src/test/org/apache/hadoop/hive/ql/exec/InputEstimatorTestClass.java
> PRE-CREATION
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java
> e444946e990d9adb90ce24837cfe4edcf5126d3a
>
> Diff: https://reviews.apache.org/r/55994/diff/
>
>
> Testing
> -------
>
> Waiting for tests HiveQA
>
>
> Thanks,
>
> Sergio Pena
>
>
