Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-15 Thread Dmytro Sen 

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

(Updated Июнь 15, 2016, 2 п.п.)


Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
Brodetskyi.


Bugs: AMBARI-17146
https://issues.apache.org/jira/browse/AMBARI-17146


Repository: ambari


Description
---

Implement config values trimming for deployment via blueprint as we do in UI

  trimProperty: function (property) {
var displayType = Em.get(property, 'displayType');
var value = Em.get(property, 'value');
var name = Em.get(property, 'name');
var rez;
switch (displayType) {
  case 'directories':
  case 'directory':
rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
break;
  case 'host':
rez = value.trim();
break;
  case 'password':
break;
  default:
if (name == 'javax.jdo.option.ConnectionURL' || name == 
'oozie.service.JPAService.jdbc.url') {
  rez = value.trim();
}
rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : value;
}
return ((rez == '') || (rez == undefined)) ? value : rez;
  },


Diffs (updated)
-

  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
 9094698 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultTrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DeleteSpacesAtTheEndTrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DirectoriesTrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PasswordTrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PropertyValueTrimmingStrategyDefiner.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
 ad8d4f9 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/TrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
 cda8fb8 
  
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackTest.java
 e70af3e 

Diff: https://reviews.apache.org/r/48494/diff/


Testing
---

Unit tests and manual tests passed


Thanks,

Dmytro Sen

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-15 Thread Daniel Gergely 

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


Fix it, then Ship it!





ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DeleteSpacesAtTheEndTrimmingStrategy.java
 (line 24)


Please swap variable and constant.


- Daniel Gergely


On jún. 15, 2016, 1:30 du, Dmytro Sen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48494/
> ---
> 
> (Updated jún. 15, 2016, 1:30 du)
> 
> 
> Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
> Brodetskyi.
> 
> 
> Bugs: AMBARI-17146
> https://issues.apache.org/jira/browse/AMBARI-17146
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Implement config values trimming for deployment via blueprint as we do in UI
> 
>   trimProperty: function (property) {
> var displayType = Em.get(property, 'displayType');
> var value = Em.get(property, 'value');
> var name = Em.get(property, 'name');
> var rez;
> switch (displayType) {
>   case 'directories':
>   case 'directory':
> rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
> break;
>   case 'host':
> rez = value.trim();
> break;
>   case 'password':
> break;
>   default:
> if (name == 'javax.jdo.option.ConnectionURL' || name == 
> 'oozie.service.JPAService.jdbc.url') {
>   rez = value.trim();
> }
> rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : 
> value;
> }
> return ((rez == '') || (rez == undefined)) ? value : rez;
>   },
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  9094698 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DeleteSpacesAtTheEndTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DirectoriesTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PasswordTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PropertyValueTrimmingStrategyDefiner.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
>  ad8d4f9 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/TrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  cda8fb8 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackTest.java
>  e70af3e 
> 
> Diff: https://reviews.apache.org/r/48494/diff/
> 
> 
> Testing
> ---
> 
> Unit tests and manual tests passed
> 
> 
> Thanks,
> 
> Dmytro Sen
> 
>

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-15 Thread Dmytro Sen 

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

(Updated Июнь 15, 2016, 1:30 п.п.)


Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
Brodetskyi.


Bugs: AMBARI-17146
https://issues.apache.org/jira/browse/AMBARI-17146


Repository: ambari


Description
---

Implement config values trimming for deployment via blueprint as we do in UI

  trimProperty: function (property) {
var displayType = Em.get(property, 'displayType');
var value = Em.get(property, 'value');
var name = Em.get(property, 'name');
var rez;
switch (displayType) {
  case 'directories':
  case 'directory':
rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
break;
  case 'host':
rez = value.trim();
break;
  case 'password':
break;
  default:
if (name == 'javax.jdo.option.ConnectionURL' || name == 
'oozie.service.JPAService.jdbc.url') {
  rez = value.trim();
}
rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : value;
}
return ((rez == '') || (rez == undefined)) ? value : rez;
  },


Diffs (updated)
-

  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
 9094698 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultTrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DeleteSpacesAtTheEndTrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DirectoriesTrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PasswordTrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PropertyValueTrimmingStrategyDefiner.java
 PRE-CREATION 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
 ad8d4f9 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/TrimmingStrategy.java
 PRE-CREATION 
  
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
 cda8fb8 
  
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackTest.java
 e70af3e 

Diff: https://reviews.apache.org/r/48494/diff/


Testing
---

Unit tests and manual tests passed


Thanks,

Dmytro Sen

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-15 Thread Daniel Gergely 

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




ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
 (lines 364 - 367)


Since there is no computational expensive calculation in this block, the if 
statement can be omitted. isDebugEnabled is used when logging the message needs 
some calculation that is used by the debug logging only.



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DirectoriesTrimmingStrategy.java
 (line 25)


Is it possible that an element in the comma separated string contains space?

If it is so, then maybe this whole function could be replaced by:
return stringToTrim.replaceAll("\\s*,+\\s*", ",");
(replacing sequences containing at least one comma and spaces around them 
to a single comma. It also removes trailing and leading commas, but preserves 
spaces within the elements. You can try it on http://regex101.com)



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DirectoriesTrimmingStrategy.java
 (lines 26 - 33)


If the regex in my previuos comment cannot be used, then you can use 
StringUtils to simplify this part:
return StringUtils.join(stringsArray, ",");



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PropertyValueTrimmingStrategyDefiner.java
 (line 41)


Simplify this by swapping the constant and the variable (you can use it as 
a thumbrule)
if("directory".equals(type) || "directories".equals(type))



ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PropertyValueTrimmingStrategyDefiner.java
 (line 43)


Swap constant and variable


- Daniel Gergely


On jún. 14, 2016, 1:45 du, Dmytro Sen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48494/
> ---
> 
> (Updated jún. 14, 2016, 1:45 du)
> 
> 
> Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
> Brodetskyi.
> 
> 
> Bugs: AMBARI-17146
> https://issues.apache.org/jira/browse/AMBARI-17146
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Implement config values trimming for deployment via blueprint as we do in UI
> 
>   trimProperty: function (property) {
> var displayType = Em.get(property, 'displayType');
> var value = Em.get(property, 'value');
> var name = Em.get(property, 'name');
> var rez;
> switch (displayType) {
>   case 'directories':
>   case 'directory':
> rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
> break;
>   case 'host':
> rez = value.trim();
> break;
>   case 'password':
> break;
>   default:
> if (name == 'javax.jdo.option.ConnectionURL' || name == 
> 'oozie.service.JPAService.jdbc.url') {
>   rez = value.trim();
> }
> rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : 
> value;
> }
> return ((rez == '') || (rez == undefined)) ? value : rez;
>   },
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  9094698 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DeleteSpacesAtTheEndTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DirectoriesTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PasswordTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PropertyValueTrimmingStrategyDefiner.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
>  ad8d4f9 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/TrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  cda8fb8 
> 
> Diff: https://reviews.apache.org/r/48494/diff/
> 
> 
> Testing
> ---
> 
> Unit tests and manual tests passed
> 
> 
> Thanks,
> 
> Dmytro Sen
> 
>

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-15 Thread Sebastian Toader 

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


Ship it!




Ship It!

- Sebastian Toader


On June 14, 2016, 3:45 p.m., Dmytro Sen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48494/
> ---
> 
> (Updated June 14, 2016, 3:45 p.m.)
> 
> 
> Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
> Brodetskyi.
> 
> 
> Bugs: AMBARI-17146
> https://issues.apache.org/jira/browse/AMBARI-17146
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Implement config values trimming for deployment via blueprint as we do in UI
> 
>   trimProperty: function (property) {
> var displayType = Em.get(property, 'displayType');
> var value = Em.get(property, 'value');
> var name = Em.get(property, 'name');
> var rez;
> switch (displayType) {
>   case 'directories':
>   case 'directory':
> rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
> break;
>   case 'host':
> rez = value.trim();
> break;
>   case 'password':
> break;
>   default:
> if (name == 'javax.jdo.option.ConnectionURL' || name == 
> 'oozie.service.JPAService.jdbc.url') {
>   rez = value.trim();
> }
> rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : 
> value;
> }
> return ((rez == '') || (rez == undefined)) ? value : rez;
>   },
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  9094698 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DefaultTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DeleteSpacesAtTheEndTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/DirectoriesTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PasswordTrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/PropertyValueTrimmingStrategyDefiner.java
>  PRE-CREATION 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
>  ad8d4f9 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/TrimmingStrategy.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  cda8fb8 
> 
> Diff: https://reviews.apache.org/r/48494/diff/
> 
> 
> Testing
> ---
> 
> Unit tests and manual tests passed
> 
> 
> Thanks,
> 
> Dmytro Sen
> 
>

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-10 Thread Dmytro Sen 


> On Июнь 9, 2016, 4:05 п.п., Daniel Gergely wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java,
> >  line 343
> > 
> >
> > Please consider cyclomatic complexity in this method.
> 
> Dmytro Sen wrote:
> What is wrong with it ? The loop body is executed only once for every 
> property. 
> Moreover, this method is executed only once during ambari server 
> lifecycle, when the server is least loaded.
> 
> Daniel Gergely wrote:
> Loops have no relations with cyclomatic complexity. I think you wrote 
> about computational complexity (more specifically time complexity). 
> Cyclomatic complexity is a measure to show how many possible execution paths 
> are there in a specific block (eg in a method body or in a class). Each of 
> the **if** statements increases this number, since there are always 2 
> options: true or false.
> I counted 11 levels of deepness here, most of them are if statements, 
> which means a high cyclomatic complexity. 
> My review was only about avoiding it, because it makes the code less 
> maintainable and readable. Computational complexity won't be less, business 
> logic won't be changed, what you gain is maintainibility and in some cases it 
> can be easier to test.
> There are many ways how to reduce it, usually some code can be simplified 
> or a new method can be introduced. (if you google it up, you will find many 
> examples. First I would extract methods.)
> This is only suggestion, to avoid bad moments for the developer who may 
> need to change the code in the future.

Thanks for detailed clarification. 
I'll update the patch.


- Dmytro


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


On Июнь 9, 2016, 3:23 п.п., Dmytro Sen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48494/
> ---
> 
> (Updated Июнь 9, 2016, 3:23 п.п.)
> 
> 
> Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
> Brodetskyi.
> 
> 
> Bugs: AMBARI-17146
> https://issues.apache.org/jira/browse/AMBARI-17146
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Implement config values trimming for deployment via blueprint as we do in UI
> 
>   trimProperty: function (property) {
> var displayType = Em.get(property, 'displayType');
> var value = Em.get(property, 'value');
> var name = Em.get(property, 'name');
> var rez;
> switch (displayType) {
>   case 'directories':
>   case 'directory':
> rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
> break;
>   case 'host':
> rez = value.trim();
> break;
>   case 'password':
> break;
>   default:
> if (name == 'javax.jdo.option.ConnectionURL' || name == 
> 'oozie.service.JPAService.jdbc.url') {
>   rez = value.trim();
> }
> rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : 
> value;
> }
> return ((rez == '') || (rez == undefined)) ? value : rez;
>   },
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  de70a2c 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
>  ad8d4f9 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  9ec0a09 
> 
> Diff: https://reviews.apache.org/r/48494/diff/
> 
> 
> Testing
> ---
> 
> Unit tests and manual tests passed
> 
> 
> Thanks,
> 
> Dmytro Sen
> 
>

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-10 Thread Daniel Gergely 


> On jún. 9, 2016, 4:05 du, Daniel Gergely wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java,
> >  line 343
> > 
> >
> > Please consider cyclomatic complexity in this method.
> 
> Dmytro Sen wrote:
> What is wrong with it ? The loop body is executed only once for every 
> property. 
> Moreover, this method is executed only once during ambari server 
> lifecycle, when the server is least loaded.

Loops have no relations with cyclomatic complexity. I think you wrote about 
computational complexity (more specifically time complexity). Cyclomatic 
complexity is a measure to show how many possible execution paths are there in 
a specific block (eg in a method body or in a class). Each of the **if** 
statements increases this number, since there are always 2 options: true or 
false.
I counted 11 levels of deepness here, most of them are if statements, which 
means a high cyclomatic complexity. 
My review was only about avoiding it, because it makes the code less 
maintainable and readable. Computational complexity won't be less, business 
logic won't be changed, what you gain is maintainibility and in some cases it 
can be easier to test.
There are many ways how to reduce it, usually some code can be simplified or a 
new method can be introduced. (if you google it up, you will find many 
examples. First I would extract methods.)
This is only suggestion, to avoid bad moments for the developer who may need to 
change the code in the future.


- Daniel


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


On jún. 9, 2016, 3:23 du, Dmytro Sen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48494/
> ---
> 
> (Updated jún. 9, 2016, 3:23 du)
> 
> 
> Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
> Brodetskyi.
> 
> 
> Bugs: AMBARI-17146
> https://issues.apache.org/jira/browse/AMBARI-17146
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Implement config values trimming for deployment via blueprint as we do in UI
> 
>   trimProperty: function (property) {
> var displayType = Em.get(property, 'displayType');
> var value = Em.get(property, 'value');
> var name = Em.get(property, 'name');
> var rez;
> switch (displayType) {
>   case 'directories':
>   case 'directory':
> rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
> break;
>   case 'host':
> rez = value.trim();
> break;
>   case 'password':
> break;
>   default:
> if (name == 'javax.jdo.option.ConnectionURL' || name == 
> 'oozie.service.JPAService.jdbc.url') {
>   rez = value.trim();
> }
> rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : 
> value;
> }
> return ((rez == '') || (rez == undefined)) ? value : rez;
>   },
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  de70a2c 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
>  ad8d4f9 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  9ec0a09 
> 
> Diff: https://reviews.apache.org/r/48494/diff/
> 
> 
> Testing
> ---
> 
> Unit tests and manual tests passed
> 
> 
> Thanks,
> 
> Dmytro Sen
> 
>

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-09 Thread Dmytro Sen 


> On Июнь 9, 2016, 4:26 п.п., Sebastian Toader wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java,
> >  line 343
> > 
> >
> > Maybe we should think here a generic solution as in the future there 
> > might be new custom 'trimming' cases to handle. I thinking something like 
> > factoring out the various 'trimming strategies' into separate methods. 
> > Factor out into methods the logic that indetifies what the type properties 
> > and map what trimming strategy to be used for a property.

Yes. We can do that. The important point here is to have it in line with UI 
implementation.


- Dmytro


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


On Июнь 9, 2016, 3:23 п.п., Dmytro Sen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48494/
> ---
> 
> (Updated Июнь 9, 2016, 3:23 п.п.)
> 
> 
> Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
> Brodetskyi.
> 
> 
> Bugs: AMBARI-17146
> https://issues.apache.org/jira/browse/AMBARI-17146
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Implement config values trimming for deployment via blueprint as we do in UI
> 
>   trimProperty: function (property) {
> var displayType = Em.get(property, 'displayType');
> var value = Em.get(property, 'value');
> var name = Em.get(property, 'name');
> var rez;
> switch (displayType) {
>   case 'directories':
>   case 'directory':
> rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
> break;
>   case 'host':
> rez = value.trim();
> break;
>   case 'password':
> break;
>   default:
> if (name == 'javax.jdo.option.ConnectionURL' || name == 
> 'oozie.service.JPAService.jdbc.url') {
>   rez = value.trim();
> }
> rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : 
> value;
> }
> return ((rez == '') || (rez == undefined)) ? value : rez;
>   },
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  de70a2c 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
>  ad8d4f9 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  9ec0a09 
> 
> Diff: https://reviews.apache.org/r/48494/diff/
> 
> 
> Testing
> ---
> 
> Unit tests and manual tests passed
> 
> 
> Thanks,
> 
> Dmytro Sen
> 
>

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-09 Thread Sebastian Toader 

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




ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
 (line 343)


Maybe we should think here a generic solution as in the future there might 
be new custom 'trimming' cases to handle. I thinking something like factoring 
out the various 'trimming strategies' into separate methods. Factor out into 
methods the logic that indetifies what the type properties and map what 
trimming strategy to be used for a property.


- Sebastian Toader


On June 9, 2016, 5:23 p.m., Dmytro Sen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48494/
> ---
> 
> (Updated June 9, 2016, 5:23 p.m.)
> 
> 
> Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
> Brodetskyi.
> 
> 
> Bugs: AMBARI-17146
> https://issues.apache.org/jira/browse/AMBARI-17146
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Implement config values trimming for deployment via blueprint as we do in UI
> 
>   trimProperty: function (property) {
> var displayType = Em.get(property, 'displayType');
> var value = Em.get(property, 'value');
> var name = Em.get(property, 'name');
> var rez;
> switch (displayType) {
>   case 'directories':
>   case 'directory':
> rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
> break;
>   case 'host':
> rez = value.trim();
> break;
>   case 'password':
> break;
>   default:
> if (name == 'javax.jdo.option.ConnectionURL' || name == 
> 'oozie.service.JPAService.jdbc.url') {
>   rez = value.trim();
> }
> rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : 
> value;
> }
> return ((rez == '') || (rez == undefined)) ? value : rez;
>   },
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  de70a2c 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
>  ad8d4f9 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  9ec0a09 
> 
> Diff: https://reviews.apache.org/r/48494/diff/
> 
> 
> Testing
> ---
> 
> Unit tests and manual tests passed
> 
> 
> Thanks,
> 
> Dmytro Sen
> 
>

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-09 Thread Dmytro Sen 


> On Июнь 9, 2016, 4:05 п.п., Daniel Gergely wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java,
> >  line 343
> > 
> >
> > Please consider cyclomatic complexity in this method.

What is wrong with it ? The loop body is executed only once for every property. 
Moreover, this method is executed only once during ambari server lifecycle, 
when the server is least loaded.


- Dmytro


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


On Июнь 9, 2016, 3:23 п.п., Dmytro Sen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48494/
> ---
> 
> (Updated Июнь 9, 2016, 3:23 п.п.)
> 
> 
> Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
> Brodetskyi.
> 
> 
> Bugs: AMBARI-17146
> https://issues.apache.org/jira/browse/AMBARI-17146
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Implement config values trimming for deployment via blueprint as we do in UI
> 
>   trimProperty: function (property) {
> var displayType = Em.get(property, 'displayType');
> var value = Em.get(property, 'value');
> var name = Em.get(property, 'name');
> var rez;
> switch (displayType) {
>   case 'directories':
>   case 'directory':
> rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
> break;
>   case 'host':
> rez = value.trim();
> break;
>   case 'password':
> break;
>   default:
> if (name == 'javax.jdo.option.ConnectionURL' || name == 
> 'oozie.service.JPAService.jdbc.url') {
>   rez = value.trim();
> }
> rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : 
> value;
> }
> return ((rez == '') || (rez == undefined)) ? value : rez;
>   },
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  de70a2c 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
>  ad8d4f9 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  9ec0a09 
> 
> Diff: https://reviews.apache.org/r/48494/diff/
> 
> 
> Testing
> ---
> 
> Unit tests and manual tests passed
> 
> 
> Thanks,
> 
> Dmytro Sen
> 
>

Re: Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-09 Thread Daniel Gergely 

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




ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
 (line 343)


Please consider cyclomatic complexity in this method.


- Daniel Gergely


On jún. 9, 2016, 3:23 du, Dmytro Sen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48494/
> ---
> 
> (Updated jún. 9, 2016, 3:23 du)
> 
> 
> Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
> Brodetskyi.
> 
> 
> Bugs: AMBARI-17146
> https://issues.apache.org/jira/browse/AMBARI-17146
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Implement config values trimming for deployment via blueprint as we do in UI
> 
>   trimProperty: function (property) {
> var displayType = Em.get(property, 'displayType');
> var value = Em.get(property, 'value');
> var name = Em.get(property, 'name');
> var rez;
> switch (displayType) {
>   case 'directories':
>   case 'directory':
> rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
> break;
>   case 'host':
> rez = value.trim();
> break;
>   case 'password':
> break;
>   default:
> if (name == 'javax.jdo.option.ConnectionURL' || name == 
> 'oozie.service.JPAService.jdbc.url') {
>   rez = value.trim();
> }
> rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : 
> value;
> }
> return ((rez == '') || (rez == undefined)) ? value : rez;
>   },
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
>  de70a2c 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
>  ad8d4f9 
>   
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
>  9ec0a09 
> 
> Diff: https://reviews.apache.org/r/48494/diff/
> 
> 
> Testing
> ---
> 
> Unit tests and manual tests passed
> 
> 
> Thanks,
> 
> Dmytro Sen
> 
>

Review Request 48494: Implement config values trimming for deployment via blueprint

2016-06-09 Thread Dmytro Sen 

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

Review request for Ambari, Robert Nettleton, Sebastian Toader, and Vitalyi 
Brodetskyi.


Bugs: AMBARI-17146
https://issues.apache.org/jira/browse/AMBARI-17146


Repository: ambari


Description
---

Implement config values trimming for deployment via blueprint as we do in UI

  trimProperty: function (property) {
var displayType = Em.get(property, 'displayType');
var value = Em.get(property, 'value');
var name = Em.get(property, 'name');
var rez;
switch (displayType) {
  case 'directories':
  case 'directory':
rez = value.replace(/,/g, ' ').trim().split(/\s+/g).join(',');
break;
  case 'host':
rez = value.trim();
break;
  case 'password':
break;
  default:
if (name == 'javax.jdo.option.ConnectionURL' || name == 
'oozie.service.JPAService.jdbc.url') {
  rez = value.trim();
}
rez = (typeof value == 'string') ? value.replace(/(\s+$)/g, '') : value;
}
return ((rez == '') || (rez == undefined)) ? value : rez;
  },


Diffs
-

  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java
 de70a2c 
  
ambari-server/src/main/java/org/apache/ambari/server/controller/internal/Stack.java
 ad8d4f9 
  
ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java
 9ec0a09 

Diff: https://reviews.apache.org/r/48494/diff/


Testing
---

Unit tests and manual tests passed


Thanks,

Dmytro Sen