Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-07 Thread Laszlo Puskas 

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

(Updated Sept. 7, 2016, 2:15 p.m.)


Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
Toader.


Changes
---

Added commit.


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


Repository: ambari


Description
---

Configuration changes are propagated to host components by issuing a restart of 
the affected components.
In case of master components this action will start the affected components 
(regardless it is in INSTALLED or STARTED state) and the desired state will be 
set to STARTED.
In Ambari client components are always expected to be in INSTALLED state. 
However upon configuration changes, the desired state of these components are 
also set to STARTED. This may be misleading for ambari API users that determine 
component states by checking the desired state and actual state. (The actual 
state in case of client components will always be INSTALLED)
This issue addresses this problem by not updating the desired state when client 
components are restarted.


Diffs
-

  
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
 8c8ae10 

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


Testing (updated)
---

Manually done.

commit 6d13be5b9ab6aa129bc1a71fc2eb57ccf7bbe6cd
Author: Toader, Sebastian 
Date:?? Wed Sep 7 15:18:25 2016 +0200


Thanks,

Laszlo Puskas

Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-07 Thread Laszlo Puskas 

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


Ship it!




Ship It!

- Laszlo Puskas


On Sept. 7, 2016, 2:15 p.m., Laszlo Puskas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51597/
> ---
> 
> (Updated Sept. 7, 2016, 2:15 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
> Toader.
> 
> 
> Bugs: AMBARI-18302
> https://issues.apache.org/jira/browse/AMBARI-18302
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Configuration changes are propagated to host components by issuing a restart 
> of the affected components.
> In case of master components this action will start the affected components 
> (regardless it is in INSTALLED or STARTED state) and the desired state will 
> be set to STARTED.
> In Ambari client components are always expected to be in INSTALLED state. 
> However upon configuration changes, the desired state of these components are 
> also set to STARTED. This may be misleading for ambari API users that 
> determine component states by checking the desired state and actual state. 
> (The actual state in case of client components will always be INSTALLED)
> This issue addresses this problem by not updating the desired state when 
> client components are restarted.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
>  8c8ae10 
> 
> Diff: https://reviews.apache.org/r/51597/diff/
> 
> 
> Testing
> ---
> 
> Manually done.
> 
> commit 6d13be5b9ab6aa129bc1a71fc2eb57ccf7bbe6cd
> Author: Toader, Sebastian 
> Date:?? Wed Sep 7 15:18:25 2016 +0200
> 
> 
> Thanks,
> 
> Laszlo Puskas
> 
>

Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-07 Thread Laszlo Puskas 


> On Sept. 2, 2016, 2:22 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java,
> >  lines 473-478
> > 
> >
> > I'm curious if we should also try to catch this in the 
> > ServiceComponentHost; don't let it's state move to STARTED if it knows it's 
> > a client.
> 
> Laszlo Puskas wrote:
> Good point, i am a bit uncertain though that such thing goes into that 
> class. (I'd expect some kind of state machine or other specialized service 
> implementation to decide which are the valid state transitions for 
> hostcomponents.) As this change only affects custom commands i'd leave this 
> as it is; please let me know if this is acceptable.
> 
> Jonathan Hurley wrote:
> I don't think it should hold up this review as you've fixed the specific 
> issue with the command. I also agree that there should be a state machine to 
> govern this. The problem is that the low budget state machine we currently 
> use doesn't take into account the type of component which the state is being 
> added for. Catching it in the impl will ensure that another mistake can't be 
> made. Having a client in the STARTED state could really mess with other logic 
> in the system.

I am about to register this in a bugdb item.


- Laszlo


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


On Sept. 2, 2016, 12:11 p.m., Laszlo Puskas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51597/
> ---
> 
> (Updated Sept. 2, 2016, 12:11 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
> Toader.
> 
> 
> Bugs: AMBARI-18302
> https://issues.apache.org/jira/browse/AMBARI-18302
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Configuration changes are propagated to host components by issuing a restart 
> of the affected components.
> In case of master components this action will start the affected components 
> (regardless it is in INSTALLED or STARTED state) and the desired state will 
> be set to STARTED.
> In Ambari client components are always expected to be in INSTALLED state. 
> However upon configuration changes, the desired state of these components are 
> also set to STARTED. This may be misleading for ambari API users that 
> determine component states by checking the desired state and actual state. 
> (The actual state in case of client components will always be INSTALLED)
> This issue addresses this problem by not updating the desired state when 
> client components are restarted.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
>  8c8ae10 
> 
> Diff: https://reviews.apache.org/r/51597/diff/
> 
> 
> Testing
> ---
> 
> Unit tests running.
> 
> 
> Thanks,
> 
> Laszlo Puskas
> 
>

Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-06 Thread Jonathan Hurley 


> On Sept. 2, 2016, 10:22 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java,
> >  lines 473-478
> > 
> >
> > I'm curious if we should also try to catch this in the 
> > ServiceComponentHost; don't let it's state move to STARTED if it knows it's 
> > a client.
> 
> Laszlo Puskas wrote:
> Good point, i am a bit uncertain though that such thing goes into that 
> class. (I'd expect some kind of state machine or other specialized service 
> implementation to decide which are the valid state transitions for 
> hostcomponents.) As this change only affects custom commands i'd leave this 
> as it is; please let me know if this is acceptable.

I don't think it should hold up this review as you've fixed the specific issue 
with the command. I also agree that there should be a state machine to govern 
this. The problem is that the low budget state machine we currently use doesn't 
take into account the type of component which the state is being added for. 
Catching it in the impl will ensure that another mistake can't be made. Having 
a client in the STARTED state could really mess with other logic in the system.


- Jonathan


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


On Sept. 2, 2016, 8:11 a.m., Laszlo Puskas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51597/
> ---
> 
> (Updated Sept. 2, 2016, 8:11 a.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
> Toader.
> 
> 
> Bugs: AMBARI-18302
> https://issues.apache.org/jira/browse/AMBARI-18302
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Configuration changes are propagated to host components by issuing a restart 
> of the affected components.
> In case of master components this action will start the affected components 
> (regardless it is in INSTALLED or STARTED state) and the desired state will 
> be set to STARTED.
> In Ambari client components are always expected to be in INSTALLED state. 
> However upon configuration changes, the desired state of these components are 
> also set to STARTED. This may be misleading for ambari API users that 
> determine component states by checking the desired state and actual state. 
> (The actual state in case of client components will always be INSTALLED)
> This issue addresses this problem by not updating the desired state when 
> client components are restarted.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
>  8c8ae10 
> 
> Diff: https://reviews.apache.org/r/51597/diff/
> 
> 
> Testing
> ---
> 
> Unit tests running.
> 
> 
> Thanks,
> 
> Laszlo Puskas
> 
>

Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-06 Thread Sandor Magyari 

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


Ship it!




Ship It!

- Sandor Magyari


On Sept. 2, 2016, 12:11 p.m., Laszlo Puskas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51597/
> ---
> 
> (Updated Sept. 2, 2016, 12:11 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
> Toader.
> 
> 
> Bugs: AMBARI-18302
> https://issues.apache.org/jira/browse/AMBARI-18302
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Configuration changes are propagated to host components by issuing a restart 
> of the affected components.
> In case of master components this action will start the affected components 
> (regardless it is in INSTALLED or STARTED state) and the desired state will 
> be set to STARTED.
> In Ambari client components are always expected to be in INSTALLED state. 
> However upon configuration changes, the desired state of these components are 
> also set to STARTED. This may be misleading for ambari API users that 
> determine component states by checking the desired state and actual state. 
> (The actual state in case of client components will always be INSTALLED)
> This issue addresses this problem by not updating the desired state when 
> client components are restarted.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
>  8c8ae10 
> 
> Diff: https://reviews.apache.org/r/51597/diff/
> 
> 
> Testing
> ---
> 
> Unit tests running.
> 
> 
> Thanks,
> 
> Laszlo Puskas
> 
>

Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-06 Thread Sebastian Toader 

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


Ship it!




Ship It!

- Sebastian Toader


On Sept. 2, 2016, 2:11 p.m., Laszlo Puskas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51597/
> ---
> 
> (Updated Sept. 2, 2016, 2:11 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
> Toader.
> 
> 
> Bugs: AMBARI-18302
> https://issues.apache.org/jira/browse/AMBARI-18302
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Configuration changes are propagated to host components by issuing a restart 
> of the affected components.
> In case of master components this action will start the affected components 
> (regardless it is in INSTALLED or STARTED state) and the desired state will 
> be set to STARTED.
> In Ambari client components are always expected to be in INSTALLED state. 
> However upon configuration changes, the desired state of these components are 
> also set to STARTED. This may be misleading for ambari API users that 
> determine component states by checking the desired state and actual state. 
> (The actual state in case of client components will always be INSTALLED)
> This issue addresses this problem by not updating the desired state when 
> client components are restarted.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
>  8c8ae10 
> 
> Diff: https://reviews.apache.org/r/51597/diff/
> 
> 
> Testing
> ---
> 
> Unit tests running.
> 
> 
> Thanks,
> 
> Laszlo Puskas
> 
>

Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-05 Thread Laszlo Puskas 


> On Sept. 2, 2016, 2:22 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java,
> >  lines 473-478
> > 
> >
> > I'm curious if we should also try to catch this in the 
> > ServiceComponentHost; don't let it's state move to STARTED if it knows it's 
> > a client.

Good point, i am a bit uncertain though that such thing goes into that class. 
(I'd expect some kind of state machine or other specialized service 
implementation to decide which are the valid state transitions for 
hostcomponents.) As this change only affects custom commands i'd leave this as 
it is; please let me know if this is acceptable.


- Laszlo


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


On Sept. 2, 2016, 12:11 p.m., Laszlo Puskas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51597/
> ---
> 
> (Updated Sept. 2, 2016, 12:11 p.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
> Toader.
> 
> 
> Bugs: AMBARI-18302
> https://issues.apache.org/jira/browse/AMBARI-18302
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Configuration changes are propagated to host components by issuing a restart 
> of the affected components.
> In case of master components this action will start the affected components 
> (regardless it is in INSTALLED or STARTED state) and the desired state will 
> be set to STARTED.
> In Ambari client components are always expected to be in INSTALLED state. 
> However upon configuration changes, the desired state of these components are 
> also set to STARTED. This may be misleading for ambari API users that 
> determine component states by checking the desired state and actual state. 
> (The actual state in case of client components will always be INSTALLED)
> This issue addresses this problem by not updating the desired state when 
> client components are restarted.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
>  8c8ae10 
> 
> Diff: https://reviews.apache.org/r/51597/diff/
> 
> 
> Testing
> ---
> 
> Unit tests running.
> 
> 
> Thanks,
> 
> Laszlo Puskas
> 
>

Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-02 Thread Jonathan Hurley 

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




ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
 (lines 472 - 477)


I'm curious if we should also try to catch this in the 
ServiceComponentHost; don't let it's state move to STARTED if it knows it's a 
client.


- Jonathan Hurley


On Sept. 2, 2016, 8:11 a.m., Laszlo Puskas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51597/
> ---
> 
> (Updated Sept. 2, 2016, 8:11 a.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
> Toader.
> 
> 
> Bugs: AMBARI-18302
> https://issues.apache.org/jira/browse/AMBARI-18302
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Configuration changes are propagated to host components by issuing a restart 
> of the affected components.
> In case of master components this action will start the affected components 
> (regardless it is in INSTALLED or STARTED state) and the desired state will 
> be set to STARTED.
> In Ambari client components are always expected to be in INSTALLED state. 
> However upon configuration changes, the desired state of these components are 
> also set to STARTED. This may be misleading for ambari API users that 
> determine component states by checking the desired state and actual state. 
> (The actual state in case of client components will always be INSTALLED)
> This issue addresses this problem by not updating the desired state when 
> client components are restarted.
> 
> 
> Diffs
> -
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
>  8c8ae10 
> 
> Diff: https://reviews.apache.org/r/51597/diff/
> 
> 
> Testing
> ---
> 
> Unit tests running.
> 
> 
> Thanks,
> 
> Laszlo Puskas
> 
>

Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-02 Thread Laszlo Puskas 

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

(Updated Sept. 2, 2016, 12:11 p.m.)


Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
Toader.


Changes
---

Added log message.


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


Repository: ambari


Description
---

Configuration changes are propagated to host components by issuing a restart of 
the affected components.
In case of master components this action will start the affected components 
(regardless it is in INSTALLED or STARTED state) and the desired state will be 
set to STARTED.
In Ambari client components are always expected to be in INSTALLED state. 
However upon configuration changes, the desired state of these components are 
also set to STARTED. This may be misleading for ambari API users that determine 
component states by checking the desired state and actual state. (The actual 
state in case of client components will always be INSTALLED)
This issue addresses this problem by not updating the desired state when client 
components are restarted.


Diffs (updated)
-

  
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
 8c8ae10 

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


Testing
---

Unit tests running.


Thanks,

Laszlo Puskas

Re: Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-02 Thread Laszlo Puskas 

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

(Updated Sept. 2, 2016, 12:08 p.m.)


Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
Toader.


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


Repository: ambari


Description
---

Configuration changes are propagated to host components by issuing a restart of 
the affected components.
In case of master components this action will start the affected components 
(regardless it is in INSTALLED or STARTED state) and the desired state will be 
set to STARTED.
In Ambari client components are always expected to be in INSTALLED state. 
However upon configuration changes, the desired state of these components are 
also set to STARTED. This may be misleading for ambari API users that determine 
component states by checking the desired state and actual state. (The actual 
state in case of client components will always be INSTALLED)
This issue addresses this problem by not updating the desired state when client 
components are restarted.


Diffs (updated)
-

  
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
 8c8ae10 

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


Testing
---

Unit tests running.


Thanks,

Laszlo Puskas

Review Request 51597: Desired state of client component should not be changed in case configuration changes are applied through a "Restart"

2016-09-02 Thread Laszlo Puskas 

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

Review request for Ambari, Jonathan Hurley, Sandor Magyari, and Sebastian 
Toader.


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


Repository: ambari


Description
---

Configuration changes are propagated to host components by issuing a restart of 
the affected components.
In case of master components this action will start the affected components 
(regardless it is in INSTALLED or STARTED state) and the desired state will be 
set to STARTED.
In Ambari client components are always expected to be in INSTALLED state. 
However upon configuration changes, the desired state of these components are 
also set to STARTED. This may be misleading for ambari API users that determine 
component states by checking the desired state and actual state. (The actual 
state in case of client components will always be INSTALLED)
This issue addresses this problem by not updating the desired state when client 
components are restarted.


Diffs
-

  
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelper.java
 8c8ae10 

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


Testing
---

Unit tests running.


Thanks,

Laszlo Puskas