> On Aug. 8, 2016, 10:10 a.m., Hayat Behlim wrote:
> > ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/s3/S3Util.java,
> >  line 47
> > <https://reviews.apache.org/r/50884/diff/1/?file=1465944#file1465944line47>
> >
> >     I wanted S3Util as a singleton utility that's why I have made it enum, 
> > Is there any specific reason to make it class ?
> 
> Hemanth Yamijala wrote:
>     Hayat, I needed to mock S3Util so I could test it by not needing S3 
> functionality up and running (see S3UploaderTest). This was not possible 
> through an enum. Hence the reason to make it into a class. S3Util.INSTANCE 
> still works as the previously defined ENUM version. Let me know if you see 
> any issues with this.
> 
> Hayat Behlim wrote:
>     No, There is no issue it will work as previously,It looks fine.Now I 
> understand mocking is the reason. Thanks for the clarification.
> 
> Oliver Szabo wrote:
>     anyway, its not really a Utility class. If it is then it should have only 
> small helper static methods, so i think it should be named somehow else later 
> based on their responsibilities

There are 3 methods in this class that are used externally: two to upload files 
to S3, and the other to read from S3. There are two options to go forward: We 
could either split these into separate classes like S3FileUploader and 
S3FileReader, or we could move these methods into classes which currently use 
them. The first approach allows us to stub them in unit testing, hence I prefer 
that. Some methods like getBucketName and getS3Key can be abstracted into an 
S3Path class. These seem like improvements that can be made in a refactoring 
JIRA. If that's OK, can we go ahead and commit these changes and file a new 
JIRA for modifying the S3Util class?


- Hemanth


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


On Aug. 8, 2016, 5:07 a.m., Hemanth Yamijala wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50884/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2016, 5:07 a.m.)
> 
> 
> Review request for Ambari, Hayat Behlim, Dharmesh Makwana, Miklos Gergely, 
> and Oliver Szabo.
> 
> 
> Bugs: AMBARI-17785
>     https://issues.apache.org/jira/browse/AMBARI-17785
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> This patch adds support for periodically uploading logs to S3 from logfeeder. 
> The set of changes included are:
> 
> * Supports one at a time processing of log events in the OutputS3File case. 
> These are spooled locally and uploaded periodically to S3.
> * Supports upload based on two criteria - file size threshold, and time based 
> threshold.
> * Refactors code to achieve the above, while not duplicating any existing 
> functions - for e.g. the code path to upload files all at once is still 
> retained and uses the same helper classes like S3Uploader etc.
> * Unit tests added for all new code.
> 
> What is **not** part of this patch, which I plan to cover in forthcoming 
> patches:
> 
> * Support for other security models as applicable (for e.g. using IAM role 
> based security for instances running on EC2)
> * Better resilience to errors and fault tolerance.
> * Copying configs, if required, to the S3 buckets.
> 
> 
> Diffs
> -----
> 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/input/InputMarker.java
>  8def4b9 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/OutputS3File.java
>  f42195c 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/S3LogPathResolver.java
>  PRE-CREATION 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/S3OutputConfiguration.java
>  PRE-CREATION 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/S3Uploader.java
>  PRE-CREATION 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/output/spool/LogSpooler.java
>  306326a 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/main/java/org/apache/ambari/logfeeder/s3/S3Util.java
>  ced2b5c 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/test/java/org/apache/ambari/logfeeder/output/OutputS3FileTest.java
>  PRE-CREATION 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/test/java/org/apache/ambari/logfeeder/output/S3LogPathResolverTest.java
>  PRE-CREATION 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/test/java/org/apache/ambari/logfeeder/output/S3UploaderTest.java
>  PRE-CREATION 
>   
> ambari-logsearch/ambari-logsearch-logfeeder/src/test/java/org/apache/ambari/logfeeder/output/spool/LogSpoolerTest.java
>  7d9d78a 
> 
> Diff: https://reviews.apache.org/r/50884/diff/
> 
> 
> Testing
> -------
> 
> * New UTs added for the new functionality
> * Have manually tested that uploads to S3 occur based on reaching the file 
> size threshold and time based threshold both.
> 
> 
> Thanks,
> 
> Hemanth Yamijala
> 
>

Reply via email to