ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Speidel" <jspei...@hortonworks.com>
Subject Re: Review Request 30120: Add getUpdateDirectives to org.apache.ambari.server.api.resources.ResourceDefinition and use when handling PUT requests
Date Wed, 21 Jan 2015 15:55:16 GMT

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



ambari-server/src/main/java/org/apache/ambari/server/api/services/BaseRequest.java
<https://reviews.apache.org/r/30120/#comment113445>

    it is ok to pass in a null value for ignoredProperties so you can always use the same
form of the method



ambari-server/src/main/java/org/apache/ambari/server/api/services/BaseRequest.java
<https://reviews.apache.org/r/30120/#comment113446>

    This cast isn't safe, a resource definition is free to return any collection from the
get*Directives methods.  
    
    Sorry about using a Set in the compiler, there is no need for it to be a set and should
have been a collection.  
    
    You can either replace the cast with new HashSet(ignoredProperties) or change the compiler/lexer
to take a collection and remove the need to cast.



ambari-server/src/main/java/org/apache/ambari/server/api/services/RequestFactory.java
<https://reviews.apache.org/r/30120/#comment113449>

    batchCreate = !applyEligibleDirectives(Request.Type.POST, body, uriInfo, resource);



ambari-server/src/main/java/org/apache/ambari/server/api/services/RequestFactory.java
<https://reviews.apache.org/r/30120/#comment113451>

    Confusing name for this method.  'applyDirectives' or 'parseRequestDirectives' would be
better in my opinion.  Not all query params are directives, some are actual query properties.
 Using allDirecitivesApplicable implies that the requrest contains illegal directives when
really there is not such thing as a non-applicable directive.  Either the query param is a
directive or it is something else.  The return value is indicating if query params were specified,
not whether an invalid directive was specified.


- John Speidel


On Jan. 21, 2015, 2:28 p.m., Robert Levas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30120/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2015, 2:28 p.m.)
> 
> 
> Review request for Ambari, John Speidel, Robert Nettleton, and Tom Beerbower.
> 
> 
> Bugs: AMBARI-9230
>     https://issues.apache.org/jira/browse/AMBARI-9230
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Add `getUpdateDirectives` method to `org.apache.ambari.server.api.resources.ResourceDefinition`:
> 
> ```
>   public Collection<String> getUpdateDirectives();
> ```
> 
> Add default implementation to `org.apache.ambari.server.api.resources.BaseResourceDefinition`
to return an empty Set
> 
> Update `org.apache.ambari.server.api.services.RequestFactory` to process _update directives_
like it processes _create directives_ - see `org.apache.ambari.server.api.services.RequestFactory#createPostRequest`
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/BaseResourceDefinition.java
f98779c 
>   ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceDefinition.java
7632e64 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/BaseRequest.java
7494491 
>   ambari-server/src/main/java/org/apache/ambari/server/api/services/RequestFactory.java
649e210 
>   ambari-server/src/test/java/org/apache/ambari/server/api/services/BaseRequestTest.java
27fc077 
>   ambari-server/src/test/java/org/apache/ambari/server/api/services/RequestFactoryTest.java
5c56670 
> 
> Diff: https://reviews.apache.org/r/30120/diff/
> 
> 
> Testing
> -------
> 
> #Jenkins test results:
> 
> Running org.apache.ambari.server.api.services.RequestFactoryTest
> Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.351 sec
> 
> Running org.apache.ambari.server.api.services.GetRequestTest
> Tests run: 16, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.959 sec
> 
> Running org.apache.ambari.server.api.services.PostRequestTest
> Tests run: 16, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.911 sec
> 
> Running org.apache.ambari.server.api.services.PutRequestTest
> Tests run: 16, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.902 sec
> 
> Running org.apache.ambari.server.api.services.DeleteRequestTest
> Tests run: 16, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.902 sec
> 
> Complete ambari-server tests
> Tests run: 2576, Failures: 0, Errors: 0, Skipped: 15
> 
> 
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 01:11 h
> [INFO] Finished at: 2015-01-21T13:43:07+00:00
> [INFO] Final Memory: 43M/514M
> [INFO] ------------------------------------------------------------------------
> 
> 
> Thanks,
> 
> Robert Levas
> 
>


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