> On July 26, 2016, 11:18 a.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/VariableReplacementHelper.java,
> >  line 248
> > <https://reviews.apache.org/r/50047/diff/3/?file=1452054#file1452054line248>
> >
> >     Use StringBuilder, which doesn't incur synchronization penalties

True, but Matcher.appendReplacement is defined as 
`java.util.regex.Matcher#appendReplacement(StringBuffer sb, String 
replacement)`.


> On July 26, 2016, 11:18 a.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/VariableReplacementHelper.java,
> >  line 240
> > <https://reviews.apache.org/r/50047/diff/3/?file=1452054#file1452054line240>
> >
> >     Should be ReplaceValue (capitalize 'R')

I missed that... nice catch!


> On July 26, 2016, 11:18 a.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/VariableReplacementHelper.java,
> >  line 251
> > <https://reviews.apache.org/r/50047/diff/3/?file=1452054#file1452054line251>
> >
> >     May want to try/catch around these regex things in case they're bad.  
> > Log the error and return something which will be non-breaking ("" 
> > sufficient?)

I would prefer the exception to be thrown in the case so the issue can be found 
during testing. It is unlikely users will use this facility when setting the 
Kerberos descriptor... and if they do, the error should be made ovbious to 
them, rather than hide the issue with some "default" value.


- Robert


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


On July 26, 2016, 11:06 a.m., Anita Jebaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50047/
> -----------------------------------------------------------
> 
> (Updated July 26, 2016, 11:06 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Di Li, Jonathan Hurley, Nate 
> Cole, and Robert Levas.
> 
> 
> Bugs: AMBARI-17694
>     https://issues.apache.org/jira/browse/AMBARI-17694
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> When kerberos is enabled, the protocol for listeners in 
> /etc/kafka/conf/server.properties is updated from PLAINTEXT to PLAINTEXTSASL, 
> even though the Ambari UI shows otherwise
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/VariableReplacementHelper.java
>  66be3bf 
>   
> ambari-server/src/main/resources/common-services/KAFKA/0.8.1/package/scripts/kafka.py
>  ac7b0ae 
>   ambari-server/src/main/resources/common-services/KAFKA/0.9.0/kerberos.json 
> 2b1c01b 
>   
> ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/VariableReplacementHelperTest.java
>  ee2a671 
> 
> Diff: https://reviews.apache.org/r/50047/diff/
> 
> 
> Testing
> -------
> 
> Added 1 new test case,
>  Ran mvn test
> 
> 
> Thanks,
> 
> Anita Jebaraj
> 
>

Reply via email to