ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Levas" <rle...@hortonworks.com>
Subject Re: Review Request 34764: Hive View Should Support NameNode HA & Cluster association
Date Thu, 28 May 2015 13:44:23 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34764/#review85546
-----------------------------------------------------------

Ship it!



contrib/views/hive/src/main/java/org/apache/ambari/view/hive/PropertyValidator.java
<https://reviews.apache.org/r/34764/#comment137142>

    This method needs some JavaDocs.  For example, why is `null` a valid URL?  `null` seems
like an invalid URL to me.



contrib/views/hive/src/main/java/org/apache/ambari/view/hive/PropertyValidator.java
<https://reviews.apache.org/r/34764/#comment137141>

    Does this meet the Ambari coding coventions?  Technically it is fine, but maybe it should
be converted to 
    
    ```
    if (webhdfsUrl == null) {
      return true;
    }
    ```
    
    Note: I did notice this a few times this patch, but those instances were pre-exising code
so I didn't flag them.
    
    If there are other issues to fix AND if this notation is not widely used in views code,
I suggest to convert it (and the other instances) to Ambari standards.



contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/jobs/atsJobs/ATSRequestsDelegateImpl.java
<https://reviews.apache.org/r/34764/#comment137143>

    It seems odd to have to quote (") a value in querystring.  Are we sure this is correct?
 If so, maybe add a comment to explain.



contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/savedQueries/SavedQueryResourceManager.java
<https://reviews.apache.org/r/34764/#comment137145>

    This method really needs a JavaDoc comment.



contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/savedQueries/SavedQueryResourceManager.java
<https://reviews.apache.org/r/34764/#comment137144>

    `(\s+|\s?)` translates to `1 or more whitespace characters OR 0 or 1 whitspace characters`.
 Doesn't this really mean `0 or more whitespace characters` and thus `\s*` should be used
instead?


- Robert Levas


On May 28, 2015, 8:02 a.m., Erik Bergenholtz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34764/
> -----------------------------------------------------------
> 
> (Updated May 28, 2015, 8:02 a.m.)
> 
> 
> Review request for Ambari, Robert Levas and Tom Beerbower.
> 
> 
> Bugs: AMBARI-11474
>     https://issues.apache.org/jira/browse/AMBARI-11474
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Just like Files View, Hive View should support both cluster association and NameNode
HA
> 
> 
> Diffs
> -----
> 
>   contrib/views/hive/pom.xml da7e1a1 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/PropertyValidator.java
61efa49 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/client/ConnectionFactory.java
98256eb 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/files/FileService.java
860b2c6 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/jobs/JobService.java
ae0e828 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/jobs/atsJobs/ATSParser.java
6e46fee 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/jobs/atsJobs/ATSRequestsDelegateImpl.java
bd477a7 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/jobs/rm/RMParser.java
e1d4c73 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/jobs/rm/RMParserFactory.java
9733147 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/jobs/rm/RMRequestsDelegateImpl.java
43186f4 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/jobs/viewJobs/JobControllerImpl.java
eb2e141 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/savedQueries/SavedQueryResourceManager.java
01f9c9c 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/resources/savedQueries/SavedQueryService.java
f55c1fd 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/utils/FilePaginator.java
6282fc9 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/utils/HdfsApi.java cbc4d4b

>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/utils/HdfsUtil.java aeeb1b7

>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/utils/ServiceFormattedException.java
e9698c3 
>   contrib/views/hive/src/main/java/org/apache/ambari/view/hive/utils/SharedObjectsFactory.java
05ff7b6 
>   contrib/views/hive/src/main/resources/ui/hive-web/app/components/progress-widget.js
d7c3fda 
>   contrib/views/hive/src/main/resources/ui/hive-web/app/controllers/index.js 0e4ac32

>   contrib/views/hive/src/main/resources/ui/hive-web/app/controllers/index/history-query/logs.js
c75fffe 
>   contrib/views/hive/src/main/resources/ui/hive-web/app/controllers/job-progress.js 737506e

>   contrib/views/hive/src/main/resources/ui/hive-web/app/styles/app.scss 6264b9e 
>   contrib/views/hive/src/main/resources/ui/hive-web/app/utils/constants.js 70cd374 
>   contrib/views/hive/src/main/resources/ui/hive-web/app/views/visual-explain.js 1585a40

>   contrib/views/hive/src/main/resources/ui/hive-web/config/environment.js 86da93e 
>   contrib/views/hive/src/main/resources/view.xml 56967d2 
>   contrib/views/hive/src/test/java/org/apache/ambari/view/hive/BaseHiveTest.java f38d0e9

>   contrib/views/hive/src/test/java/org/apache/ambari/view/hive/resources/files/FileServiceTest.java
a5a0f48 
>   contrib/views/hive/src/test/java/org/apache/ambari/view/hive/resources/jobs/JobServiceTest.java
3c4202a 
>   contrib/views/hive/src/test/java/org/apache/ambari/view/hive/resources/savedQueries/SavedQueryResourceManagerTest.java
PRE-CREATION 
>   contrib/views/hive/src/test/java/org/apache/ambari/view/hive/resources/savedQueries/SavedQueryServiceTest.java
9b26a5b 
>   contrib/views/hive/src/test/java/org/apache/ambari/view/hive/utils/HdfsApiMock.java
bb922e2 
> 
> Diff: https://reviews.apache.org/r/34764/diff/
> 
> 
> Testing
> -------
> 
> Local Unit Testing and Deployment of View.
> 
> 
> Thanks,
> 
> Erik Bergenholtz
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message