You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@ambari.apache.org by Attila Doroszlai <ad...@hortonworks.com> on 2016/12/12 13:31:51 UTC
Review Request 54659: AMBARI-19149. Code cleanup: fallthrough
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54659/
-----------------------------------------------------------
Review request for Ambari, Jonathan Hurley, Laszlo Puskas, Nate Cole, and Sebastian Toader.
Bugs: AMBARI-19149
https://issues.apache.org/jira/browse/AMBARI-19149
Repository: ambari
Description
-------
* Fallthrough in `State` seems to be bug, please confirm.
* The rest of the change seems straightforward:
* Wrong fallthrough in `DBAccessorImpl` was on unused code path
* Wrong fallthrough in `ControllerModule` was no-op (URL and driver set to the same as previously)
* `TestActionQueue` seems to be unused, but I think fallthrough was unintentional.
* No functional change in `UserEventCreator` and `ConfigurationDirectory`
Diffs
-----
ambari-server/checkstyle.xml 81f6380e797f7e319e1172db83444ece1c4968bd
ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UserEventCreator.java bc469acef83187ef88d0d067a50a4f061436a0a8
ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java 053031bc4971bfcbef4e92ead9ec15b84b11a82f
ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java 123fa965b7dcbea92d462b0333f73e9350cd5e4f
ambari-server/src/main/java/org/apache/ambari/server/stack/ConfigurationDirectory.java cfdf9fc19724ee1dc15dd4c769873775013fafb6
ambari-server/src/main/java/org/apache/ambari/server/state/State.java c3f30c33d4ec88d8d272f83045d6d63e16381148
ambari-server/src/test/java/org/apache/ambari/server/agent/TestActionQueue.java 632750b91a12b75907fc66545d19ad473d089bcd
Diff: https://reviews.apache.org/r/54659/diff/
Testing
-------
$ mvn -am -pl ambari-server -DskipPythonTests -Drat.skip clean test
...
[INFO] --- maven-checkstyle-plugin:2.17:check (checkstyle) @ ambari-server ---
[INFO] Starting audit...
Audit done.
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] Ambari Main ....................................... SUCCESS [0.504s]
[INFO] Apache Ambari Project POM ......................... SUCCESS [0.003s]
[INFO] Ambari Views ...................................... SUCCESS [1.972s]
[INFO] utility ........................................... SUCCESS [0.996s]
[INFO] ambari-metrics .................................... SUCCESS [0.178s]
[INFO] Ambari Metrics Common ............................. SUCCESS [4.874s]
[INFO] Ambari Server ..................................... SUCCESS [23:54.809s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
Thanks,
Attila Doroszlai
Re: Review Request 54659: AMBARI-19149. Code cleanup: fallthrough
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54659/#review158854
-----------------------------------------------------------
Ship it!
Ship It!
- Jonathan Hurley
On Dec. 12, 2016, 8:34 a.m., Attila Doroszlai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54659/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 8:34 a.m.)
>
>
> Review request for Ambari, Jonathan Hurley, Laszlo Puskas, Nate Cole, and Sebastian Toader.
>
>
> Bugs: AMBARI-19149
> https://issues.apache.org/jira/browse/AMBARI-19149
>
>
> Repository: ambari
>
>
> Description
> -------
>
> * Fallthrough in `State` seems to be bug, please confirm.
> * The rest of the change seems straightforward:
> * Wrong fallthrough in `DBAccessorImpl` was on unused code path
> * Wrong fallthrough in `ControllerModule` was no-op (URL and driver set to the same as previously)
> * `TestActionQueue` seems to be unused, but I think fallthrough was unintentional.
> * No functional change in `UserEventCreator` and `ConfigurationDirectory`
>
>
> Diffs
> -----
>
> ambari-server/checkstyle.xml 81f6380e797f7e319e1172db83444ece1c4968bd
> ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UserEventCreator.java bc469acef83187ef88d0d067a50a4f061436a0a8
> ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java 053031bc4971bfcbef4e92ead9ec15b84b11a82f
> ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java 123fa965b7dcbea92d462b0333f73e9350cd5e4f
> ambari-server/src/main/java/org/apache/ambari/server/stack/ConfigurationDirectory.java cfdf9fc19724ee1dc15dd4c769873775013fafb6
> ambari-server/src/main/java/org/apache/ambari/server/state/State.java c3f30c33d4ec88d8d272f83045d6d63e16381148
> ambari-server/src/test/java/org/apache/ambari/server/agent/TestActionQueue.java 632750b91a12b75907fc66545d19ad473d089bcd
>
> Diff: https://reviews.apache.org/r/54659/diff/
>
>
> Testing
> -------
>
> $ mvn -am -pl ambari-server -DskipPythonTests -Drat.skip clean test
> ...
> [INFO] --- maven-checkstyle-plugin:2.17:check (checkstyle) @ ambari-server ---
> [INFO] Starting audit...
> Audit done.
> [INFO] ------------------------------------------------------------------------
> [INFO] Reactor Summary:
> [INFO]
> [INFO] Ambari Main ....................................... SUCCESS [0.504s]
> [INFO] Apache Ambari Project POM ......................... SUCCESS [0.003s]
> [INFO] Ambari Views ...................................... SUCCESS [1.972s]
> [INFO] utility ........................................... SUCCESS [0.996s]
> [INFO] ambari-metrics .................................... SUCCESS [0.178s]
> [INFO] Ambari Metrics Common ............................. SUCCESS [4.874s]
> [INFO] Ambari Server ..................................... SUCCESS [23:54.809s]
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
>
>
> Thanks,
>
> Attila Doroszlai
>
>
Re: Review Request 54659: AMBARI-19149. Code cleanup: fallthrough
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54659/#review158845
-----------------------------------------------------------
Fix it, then Ship it!
ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java (lines 231 - 235)
<https://reviews.apache.org/r/54659/#comment229669>
I'm really surprised by this one. We may want to keep THROW_EXCEPTIONS as true for remote. Jonathan can comment too.
- Nate Cole
On Dec. 12, 2016, 8:34 a.m., Attila Doroszlai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54659/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 8:34 a.m.)
>
>
> Review request for Ambari, Jonathan Hurley, Laszlo Puskas, Nate Cole, and Sebastian Toader.
>
>
> Bugs: AMBARI-19149
> https://issues.apache.org/jira/browse/AMBARI-19149
>
>
> Repository: ambari
>
>
> Description
> -------
>
> * Fallthrough in `State` seems to be bug, please confirm.
> * The rest of the change seems straightforward:
> * Wrong fallthrough in `DBAccessorImpl` was on unused code path
> * Wrong fallthrough in `ControllerModule` was no-op (URL and driver set to the same as previously)
> * `TestActionQueue` seems to be unused, but I think fallthrough was unintentional.
> * No functional change in `UserEventCreator` and `ConfigurationDirectory`
>
>
> Diffs
> -----
>
> ambari-server/checkstyle.xml 81f6380e797f7e319e1172db83444ece1c4968bd
> ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UserEventCreator.java bc469acef83187ef88d0d067a50a4f061436a0a8
> ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java 053031bc4971bfcbef4e92ead9ec15b84b11a82f
> ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java 123fa965b7dcbea92d462b0333f73e9350cd5e4f
> ambari-server/src/main/java/org/apache/ambari/server/stack/ConfigurationDirectory.java cfdf9fc19724ee1dc15dd4c769873775013fafb6
> ambari-server/src/main/java/org/apache/ambari/server/state/State.java c3f30c33d4ec88d8d272f83045d6d63e16381148
> ambari-server/src/test/java/org/apache/ambari/server/agent/TestActionQueue.java 632750b91a12b75907fc66545d19ad473d089bcd
>
> Diff: https://reviews.apache.org/r/54659/diff/
>
>
> Testing
> -------
>
> $ mvn -am -pl ambari-server -DskipPythonTests -Drat.skip clean test
> ...
> [INFO] --- maven-checkstyle-plugin:2.17:check (checkstyle) @ ambari-server ---
> [INFO] Starting audit...
> Audit done.
> [INFO] ------------------------------------------------------------------------
> [INFO] Reactor Summary:
> [INFO]
> [INFO] Ambari Main ....................................... SUCCESS [0.504s]
> [INFO] Apache Ambari Project POM ......................... SUCCESS [0.003s]
> [INFO] Ambari Views ...................................... SUCCESS [1.972s]
> [INFO] utility ........................................... SUCCESS [0.996s]
> [INFO] ambari-metrics .................................... SUCCESS [0.178s]
> [INFO] Ambari Metrics Common ............................. SUCCESS [4.874s]
> [INFO] Ambari Server ..................................... SUCCESS [23:54.809s]
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
>
>
> Thanks,
>
> Attila Doroszlai
>
>
Re: Review Request 54659: AMBARI-19149. Code cleanup: fallthrough
Posted by Attila Doroszlai <ad...@hortonworks.com>.
> On Dec. 12, 2016, 3:07 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java, lines 231-235
> > <https://reviews.apache.org/r/54659/diff/1/?file=1581951#file1581951line231>
> >
> > I think this fallthrough is intentional - let's fix it and duplicate the setters in the REMOTE case.
1. I'd agree that it is intentional if there were more setters in the second case, and the first case fell through in order to set those properties. But the extra setters are in the first case. It seems more like an overlook.
2. I don't think we want `setProperty(DDL_GENERATION, DROP_AND_CREATE)` in any persistent database.
3. [THROW_EXCEPTIONS](https://eclipse.org/eclipselink/api/2.6/org/eclipse/persistence/config/PersistenceUnitProperties.html#THROW_EXCEPTIONS) is true by default.
- Attila
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54659/#review158850
-----------------------------------------------------------
On Dec. 12, 2016, 2:34 p.m., Attila Doroszlai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54659/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 2:34 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley, Laszlo Puskas, Nate Cole, and Sebastian Toader.
>
>
> Bugs: AMBARI-19149
> https://issues.apache.org/jira/browse/AMBARI-19149
>
>
> Repository: ambari
>
>
> Description
> -------
>
> * Fallthrough in `State` seems to be bug, please confirm.
> * The rest of the change seems straightforward:
> * Wrong fallthrough in `DBAccessorImpl` was on unused code path
> * Wrong fallthrough in `ControllerModule` was no-op (URL and driver set to the same as previously)
> * `TestActionQueue` seems to be unused, but I think fallthrough was unintentional.
> * No functional change in `UserEventCreator` and `ConfigurationDirectory`
>
>
> Diffs
> -----
>
> ambari-server/checkstyle.xml 81f6380e797f7e319e1172db83444ece1c4968bd
> ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UserEventCreator.java bc469acef83187ef88d0d067a50a4f061436a0a8
> ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java 053031bc4971bfcbef4e92ead9ec15b84b11a82f
> ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java 123fa965b7dcbea92d462b0333f73e9350cd5e4f
> ambari-server/src/main/java/org/apache/ambari/server/stack/ConfigurationDirectory.java cfdf9fc19724ee1dc15dd4c769873775013fafb6
> ambari-server/src/main/java/org/apache/ambari/server/state/State.java c3f30c33d4ec88d8d272f83045d6d63e16381148
> ambari-server/src/test/java/org/apache/ambari/server/agent/TestActionQueue.java 632750b91a12b75907fc66545d19ad473d089bcd
>
> Diff: https://reviews.apache.org/r/54659/diff/
>
>
> Testing
> -------
>
> $ mvn -am -pl ambari-server -DskipPythonTests -Drat.skip clean test
> ...
> [INFO] --- maven-checkstyle-plugin:2.17:check (checkstyle) @ ambari-server ---
> [INFO] Starting audit...
> Audit done.
> [INFO] ------------------------------------------------------------------------
> [INFO] Reactor Summary:
> [INFO]
> [INFO] Ambari Main ....................................... SUCCESS [0.504s]
> [INFO] Apache Ambari Project POM ......................... SUCCESS [0.003s]
> [INFO] Ambari Views ...................................... SUCCESS [1.972s]
> [INFO] utility ........................................... SUCCESS [0.996s]
> [INFO] ambari-metrics .................................... SUCCESS [0.178s]
> [INFO] Ambari Metrics Common ............................. SUCCESS [4.874s]
> [INFO] Ambari Server ..................................... SUCCESS [23:54.809s]
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
>
>
> Thanks,
>
> Attila Doroszlai
>
>
Re: Review Request 54659: AMBARI-19149. Code cleanup: fallthrough
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54659/#review158850
-----------------------------------------------------------
Fix it, then Ship it!
ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java (lines 231 - 235)
<https://reviews.apache.org/r/54659/#comment229673>
I think this fallthrough is intentional - let's fix it and duplicate the setters in the REMOTE case.
- Jonathan Hurley
On Dec. 12, 2016, 8:34 a.m., Attila Doroszlai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54659/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 8:34 a.m.)
>
>
> Review request for Ambari, Jonathan Hurley, Laszlo Puskas, Nate Cole, and Sebastian Toader.
>
>
> Bugs: AMBARI-19149
> https://issues.apache.org/jira/browse/AMBARI-19149
>
>
> Repository: ambari
>
>
> Description
> -------
>
> * Fallthrough in `State` seems to be bug, please confirm.
> * The rest of the change seems straightforward:
> * Wrong fallthrough in `DBAccessorImpl` was on unused code path
> * Wrong fallthrough in `ControllerModule` was no-op (URL and driver set to the same as previously)
> * `TestActionQueue` seems to be unused, but I think fallthrough was unintentional.
> * No functional change in `UserEventCreator` and `ConfigurationDirectory`
>
>
> Diffs
> -----
>
> ambari-server/checkstyle.xml 81f6380e797f7e319e1172db83444ece1c4968bd
> ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UserEventCreator.java bc469acef83187ef88d0d067a50a4f061436a0a8
> ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java 053031bc4971bfcbef4e92ead9ec15b84b11a82f
> ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java 123fa965b7dcbea92d462b0333f73e9350cd5e4f
> ambari-server/src/main/java/org/apache/ambari/server/stack/ConfigurationDirectory.java cfdf9fc19724ee1dc15dd4c769873775013fafb6
> ambari-server/src/main/java/org/apache/ambari/server/state/State.java c3f30c33d4ec88d8d272f83045d6d63e16381148
> ambari-server/src/test/java/org/apache/ambari/server/agent/TestActionQueue.java 632750b91a12b75907fc66545d19ad473d089bcd
>
> Diff: https://reviews.apache.org/r/54659/diff/
>
>
> Testing
> -------
>
> $ mvn -am -pl ambari-server -DskipPythonTests -Drat.skip clean test
> ...
> [INFO] --- maven-checkstyle-plugin:2.17:check (checkstyle) @ ambari-server ---
> [INFO] Starting audit...
> Audit done.
> [INFO] ------------------------------------------------------------------------
> [INFO] Reactor Summary:
> [INFO]
> [INFO] Ambari Main ....................................... SUCCESS [0.504s]
> [INFO] Apache Ambari Project POM ......................... SUCCESS [0.003s]
> [INFO] Ambari Views ...................................... SUCCESS [1.972s]
> [INFO] utility ........................................... SUCCESS [0.996s]
> [INFO] ambari-metrics .................................... SUCCESS [0.178s]
> [INFO] Ambari Metrics Common ............................. SUCCESS [4.874s]
> [INFO] Ambari Server ..................................... SUCCESS [23:54.809s]
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
>
>
> Thanks,
>
> Attila Doroszlai
>
>
Re: Review Request 54659: AMBARI-19149. Code cleanup: fallthrough
Posted by Attila Doroszlai <ad...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54659/
-----------------------------------------------------------
(Updated Dec. 12, 2016, 2:34 p.m.)
Review request for Ambari, Jonathan Hurley, Laszlo Puskas, Nate Cole, and Sebastian Toader.
Bugs: AMBARI-19149
https://issues.apache.org/jira/browse/AMBARI-19149
Repository: ambari
Description
-------
* Fallthrough in `State` seems to be bug, please confirm.
* The rest of the change seems straightforward:
* Wrong fallthrough in `DBAccessorImpl` was on unused code path
* Wrong fallthrough in `ControllerModule` was no-op (URL and driver set to the same as previously)
* `TestActionQueue` seems to be unused, but I think fallthrough was unintentional.
* No functional change in `UserEventCreator` and `ConfigurationDirectory`
Diffs
-----
ambari-server/checkstyle.xml 81f6380e797f7e319e1172db83444ece1c4968bd
ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UserEventCreator.java bc469acef83187ef88d0d067a50a4f061436a0a8
ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java 053031bc4971bfcbef4e92ead9ec15b84b11a82f
ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java 123fa965b7dcbea92d462b0333f73e9350cd5e4f
ambari-server/src/main/java/org/apache/ambari/server/stack/ConfigurationDirectory.java cfdf9fc19724ee1dc15dd4c769873775013fafb6
ambari-server/src/main/java/org/apache/ambari/server/state/State.java c3f30c33d4ec88d8d272f83045d6d63e16381148
ambari-server/src/test/java/org/apache/ambari/server/agent/TestActionQueue.java 632750b91a12b75907fc66545d19ad473d089bcd
Diff: https://reviews.apache.org/r/54659/diff/
Testing
-------
$ mvn -am -pl ambari-server -DskipPythonTests -Drat.skip clean test
...
[INFO] --- maven-checkstyle-plugin:2.17:check (checkstyle) @ ambari-server ---
[INFO] Starting audit...
Audit done.
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] Ambari Main ....................................... SUCCESS [0.504s]
[INFO] Apache Ambari Project POM ......................... SUCCESS [0.003s]
[INFO] Ambari Views ...................................... SUCCESS [1.972s]
[INFO] utility ........................................... SUCCESS [0.996s]
[INFO] ambari-metrics .................................... SUCCESS [0.178s]
[INFO] Ambari Metrics Common ............................. SUCCESS [4.874s]
[INFO] Ambari Server ..................................... SUCCESS [23:54.809s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
Thanks,
Attila Doroszlai