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




ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 146)
<https://reviews.apache.org/r/48793/#comment203599>

    I don't not sure whether this is the right place to put this check? Output 
is generally only for writing. We should probably move the code to 
FetchConfigFromSolr and call it from InputMgr.init(). And also, move     
LogfeederScheduler.INSTANCE.start(); from LogFeeder.java before inputMgr.init() 
is called.
    
    More important, it should be only called if Solr Zookeeper/URL and config 
collection is provided. In offline, mode, these might not be set.



ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 149)
<https://reviews.apache.org/r/48793/#comment203593>

    Should we wait for config? Config is optional. And in some case, configs 
might not be used at all



ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 227)
<https://reviews.apache.org/r/48793/#comment203589>

    Should this be WARN? And we have been using 
LogFeederUtil.logErrorMessageByInterval to control the number of log messages



ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 240)
<https://reviews.apache.org/r/48793/#comment203588>

    We should use a constant value (or configurable), so it can be used other 
places also. We could use RETRY_INTERVAL which is in the Worker thread by 
moving to the class level



ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 254)
<https://reviews.apache.org/r/48793/#comment203592>

    Good to print the name of the field also.



ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 267)
<https://reviews.apache.org/r/48793/#comment203598>

    Should we print using LogFeederUtil.logErrorMessageByInterval and also 
remove hard coded 10 seconds?



ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 271)
<https://reviews.apache.org/r/48793/#comment203596>

    Can we put more context here? Example the query we are running and 
collection name?



ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 273)
<https://reviews.apache.org/r/48793/#comment203597>

    Should we put more info? And also print error using 
LogFeederUtil.logErrorMessageByInterval



ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 276)
<https://reviews.apache.org/r/48793/#comment203591>

    Should use LogFeederUtil.logErrorMessageByInterval if possible



ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
 (line 280)
<https://reviews.apache.org/r/48793/#comment203590>

    Same here, we could use a constant value



ambari-server/src/main/resources/common-services/LOGSEARCH/0.5.0/package/templates/logfeeder.properties.j2
 (line 21)
<https://reviews.apache.org/r/48793/#comment203595>

    Should we take this out?


- Don Bosco Durai


On June 18, 2016, 9:10 a.m., Miklos Gergely wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48793/
> -----------------------------------------------------------
> 
> (Updated June 18, 2016, 9:10 a.m.)
> 
> 
> Review request for Ambari, Don Bosco Durai, Dharmesh Makwana, Oliver Szabo, 
> Robert Nettleton, and Sumit Mohanty.
> 
> 
> Bugs: Ambari-17277
>     https://issues.apache.org/jira/browse/Ambari-17277
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> After Log Search is started it sends the filters to the Solr which describes 
> what log levels should be persisted. In the meantime Logfeeders start to push 
> their data, which is not filtered. The result is misleading, the user may 
> find it add that some specific level logs are loaded, others are not.
> 
> 
> Diffs
> -----
> 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputSolr.java
>  b14c273 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/main/resources/logfeeder.properties
>  076c09c 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/test/java/org/apache/ambari/logfeeder/output/OutputSolrTest.java
>  afbccca 
>   
> ambari-server/src/main/resources/common-services/LOGSEARCH/0.5.0/package/templates/logfeeder.properties.j2
>  6a52708 
> 
> Diff: https://reviews.apache.org/r/48793/diff/
> 
> 
> Testing
> -------
> 
> logfeeder:
> Tests run: 35, Failures: 0, Errors: 0, Skipped: 0
> 
> ambari-server
> Total run:1062
> Total errors:0
> Total failures:0
> 
> 
> Thanks,
> 
> Miklos Gergely
> 
>

Reply via email to