> 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.
>
> Sergio Pena wrote:
> 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?
About the line length - we should discuss it with the community, but I do not
think there is a real roadblock to changing it.
About the method length - you actually added a few lines to it by adding a new
if statement to the Runnable definition. I will look into the code to check if
there is a possibility to show this warning only when we first step over the
defined line numbers, since the method was too big even to begin with.
About the message length - I will see what I can do - it is the checkstyle
warning message which is printed here.
If we have more to discuss, I think we could do it offline not to pollute your
review with other stuff :)
- Peter
-----------------------------------------------------------
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
>
>
