You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@ambari.apache.org by Balázs Bence Sári <bs...@hortonworks.com> on 2017/02/10 12:21:53 UTC
Review Request 56540: Implement new DB checks for Postgres to prevent
cross-schema confusion
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/
-----------------------------------------------------------
Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
Bugs: AMBARI-19957
https://issues.apache.org/jira/browse/AMBARI-19957
Repository: ambari
Description
-------
Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
New consistency checks on server startup warn the user in such situations.
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
ambari-server/src/main/python/ambari_server_main.py 7a21333
ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
Diff: https://reviews.apache.org/r/56540/diff/
Testing
-------
- Wrote new unit tests
- Run all tests for ambari-server (all passed)
- Performed manual testing
Thanks,
Bal�zs Bence S�ri
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Sebastian Toader <st...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165119
-----------------------------------------------------------
Ship it!
Ship It!
- Sebastian Toader
On Feb. 10, 2017, 1:21 p.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 1:21 p.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
> On Feb. 14, 2017, 4:54 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/checks/IncompatibleSchemaException.java, lines 22-25
> > <https://reviews.apache.org/r/56540/diff/2/?file=1633277#file1633277line22>
> >
> > Any specific reason for a new Exception class? There's remarkable changed that isn't covered by a "regular" AmbariException.
Will revert this. Probably a remainder of a previous - since then discarded - error handling mechanism.
- Bal�zs Bence
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165527
-----------------------------------------------------------
On Feb. 14, 2017, 3:27 p.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 3:27 p.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
> ambari-server/src/main/java/org/apache/ambari/server/checks/IncompatibleSchemaException.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
> On Feb. 14, 2017, 4:54 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java, line 780
> > <https://reviews.apache.org/r/56540/diff/2/?file=1633274#file1633274line780>
> >
> > I thought Logger uses {} for the replacement token? I wonder if this ever worked.
Sure, and it works. This line does not originate from me, I simply changed LOG.error(...) to error(...).
- Bal�zs Bence
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165527
-----------------------------------------------------------
On Feb. 14, 2017, 3:27 p.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 3:27 p.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
> ambari-server/src/main/java/org/apache/ambari/server/checks/IncompatibleSchemaException.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165527
-----------------------------------------------------------
Fix it, then Ship it!
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java (line 721)
<https://reviews.apache.org/r/56540/#comment237369>
I thought Logger uses {} for the replacement token? I wonder if this ever worked.
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java (lines 30 - 36)
<https://reviews.apache.org/r/56540/#comment237370>
Pretty clear, but should doc anyway.
ambari-server/src/main/java/org/apache/ambari/server/checks/IncompatibleSchemaException.java (lines 22 - 25)
<https://reviews.apache.org/r/56540/#comment237371>
Any specific reason for a new Exception class? There's remarkable changed that isn't covered by a "regular" AmbariException.
- Nate Cole
On Feb. 14, 2017, 10:27 a.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 10:27 a.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
> ambari-server/src/main/java/org/apache/ambari/server/checks/IncompatibleSchemaException.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165523
-----------------------------------------------------------
Fix it, then Ship it!
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java (lines 85 - 107)
<https://reviews.apache.org/r/56540/#comment237362>
Can you provide some basic documentation for the methods here?
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java (lines 24 - 25)
<https://reviews.apache.org/r/56540/#comment237365>
Make this a JavaDoc comment for better visibility.
- Jonathan Hurley
On Feb. 14, 2017, 10:27 a.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 10:27 a.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
> ambari-server/src/main/java/org/apache/ambari/server/checks/IncompatibleSchemaException.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/
-----------------------------------------------------------
(Updated Feb. 15, 2017, 3:43 p.m.)
Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
Changes
-------
Fixed a few more issues.
Bugs: AMBARI-19957
https://issues.apache.org/jira/browse/AMBARI-19957
Repository: ambari
Description
-------
Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
New consistency checks on server startup warn the user in such situations.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
ambari-server/src/main/python/ambari_server_main.py 7a21333
ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
Diff: https://reviews.apache.org/r/56540/diff/
Testing
-------
- Wrote new unit tests
- Run all tests for ambari-server (all passed)
- Performed manual testing
Thanks,
Bal�zs Bence S�ri
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/
-----------------------------------------------------------
(Updated Feb. 15, 2017, 11:33 a.m.)
Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
Changes
-------
Fixed review comments.
Bugs: AMBARI-19957
https://issues.apache.org/jira/browse/AMBARI-19957
Repository: ambari
Description
-------
Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
New consistency checks on server startup warn the user in such situations.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
ambari-server/src/main/python/ambari_server_main.py 7a21333
ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
Diff: https://reviews.apache.org/r/56540/diff/
Testing
-------
- Wrote new unit tests
- Run all tests for ambari-server (all passed)
- Performed manual testing
Thanks,
Bal�zs Bence S�ri
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/
-----------------------------------------------------------
(Updated Feb. 14, 2017, 3:27 p.m.)
Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
Changes
-------
Review comments addressed
Bugs: AMBARI-19957
https://issues.apache.org/jira/browse/AMBARI-19957
Repository: ambari
Description
-------
Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
New consistency checks on server startup warn the user in such situations.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
ambari-server/src/main/java/org/apache/ambari/server/checks/IncompatibleSchemaException.java PRE-CREATION
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
ambari-server/src/main/python/ambari_server_main.py 7a21333
ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
Diff: https://reviews.apache.org/r/56540/diff/
Testing
-------
- Wrote new unit tests
- Run all tests for ambari-server (all passed)
- Performed manual testing
Thanks,
Bal�zs Bence S�ri
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
> On Feb. 10, 2017, 2:18 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java, lines 715-729
> > <https://reviews.apache.org/r/56540/diff/1/?file=1629358#file1629358line715>
> >
> > This is strange - why not make the DatabaseConsistencyCheckHelper do this exception handling/logic and call it directly instead of having yet another method call in AmbariServer? Separation of concerns would be helpful.
>
> Bal�zs Bence S�ri wrote:
> I agree with this finding. I copied existing behavior that's why it is here. I'll move it to DB checker.
Please have look at the new implementation. I simplified error handling here, however not completely removed. I left DB Consistency checker's concern to provide a check result and AmbariServer's concern to decide what to do with the check result (e.g. exit with 1 exit code if errors occured in the checks). However, I greatly simplified the overcomplicated logic there.
> On Feb. 10, 2017, 2:18 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java, lines 724-726
> > <https://reviews.apache.org/r/56540/diff/1/?file=1629358#file1629358line724>
> >
> > Use logging here, not System.out.println(...)
>
> Bal�zs Bence S�ri wrote:
> I copied existing behavior. I think the reason why it is here that the python script checks the standard out, so the intention is to make sure this line appears there, irrespectively of the logging config.
I reduced the occasions of System.out.println's in Ambari Server (a lot of them existed in the original version). I am hesitating to completely removing them as in this case the explicit intention is to make that line appear on System.out, not the application log (as the python script relies on the console output) and I think the most reliable way to achieve that is to use System.out.println.
If I try to achieve that with loggers:
1. Users may reconfigure logging, in that case the python script will miss the output.
2. Previously (not sure if it is still the case) Java server apps suffered from loggers sometimes not getting flushed when System.exit() was called.
- Bal�zs Bence
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165122
-----------------------------------------------------------
On Feb. 14, 2017, 3:27 p.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 3:27 p.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
> ambari-server/src/main/java/org/apache/ambari/server/checks/IncompatibleSchemaException.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
> On Feb. 10, 2017, 2:18 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java, lines 724-726
> > <https://reviews.apache.org/r/56540/diff/1/?file=1629358#file1629358line724>
> >
> > Use logging here, not System.out.println(...)
>
> Bal�zs Bence S�ri wrote:
> I copied existing behavior. I think the reason why it is here that the python script checks the standard out, so the intention is to make sure this line appears there, irrespectively of the logging config.
>
> Bal�zs Bence S�ri wrote:
> I reduced the occasions of System.out.println's in Ambari Server (a lot of them existed in the original version). I am hesitating to completely removing them as in this case the explicit intention is to make that line appear on System.out, not the application log (as the python script relies on the console output) and I think the most reliable way to achieve that is to use System.out.println.
> If I try to achieve that with loggers:
> 1. Users may reconfigure logging, in that case the python script will miss the output.
> 2. Previously (not sure if it is still the case) Java server apps suffered from loggers sometimes not getting flushed when System.exit() was called.
I removed my new method containing println(). Also reduced the number of printlns() in the original code as much as possible (respecting the python script).
> On Feb. 10, 2017, 2:18 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java, lines 715-729
> > <https://reviews.apache.org/r/56540/diff/1/?file=1629358#file1629358line715>
> >
> > This is strange - why not make the DatabaseConsistencyCheckHelper do this exception handling/logic and call it directly instead of having yet another method call in AmbariServer? Separation of concerns would be helpful.
>
> Bal�zs Bence S�ri wrote:
> I agree with this finding. I copied existing behavior that's why it is here. I'll move it to DB checker.
>
> Bal�zs Bence S�ri wrote:
> Please have look at the new implementation. I simplified error handling here, however not completely removed. I left DB Consistency checker's concern to provide a check result and AmbariServer's concern to decide what to do with the check result (e.g. exit with 1 exit code if errors occured in the checks). However, I greatly simplified the overcomplicated logic there.
I removed my new method and simplified the original check-result handling.
- Bal�zs Bence
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165122
-----------------------------------------------------------
On Feb. 15, 2017, 11:33 a.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 11:33 a.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
> On Feb. 10, 2017, 2:18 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java, lines 715-729
> > <https://reviews.apache.org/r/56540/diff/1/?file=1629358#file1629358line715>
> >
> > This is strange - why not make the DatabaseConsistencyCheckHelper do this exception handling/logic and call it directly instead of having yet another method call in AmbariServer? Separation of concerns would be helpful.
I agree with this finding. I copied existing behavior that's why it is here. I'll move it to DB checker.
> On Feb. 10, 2017, 2:18 p.m., Nate Cole wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java, lines 724-726
> > <https://reviews.apache.org/r/56540/diff/1/?file=1629358#file1629358line724>
> >
> > Use logging here, not System.out.println(...)
I copied existing behavior. I think the reason why it is here that the python script checks the standard out, so the intention is to make sure this line appears there, irrespectively of the logging config.
- Bal�zs Bence
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165122
-----------------------------------------------------------
On Feb. 10, 2017, 12:21 p.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 12:21 p.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165122
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java (lines 715 - 729)
<https://reviews.apache.org/r/56540/#comment236933>
This is strange - why not make the DatabaseConsistencyCheckHelper do this exception handling/logic and call it directly instead of having yet another method call in AmbariServer? Separation of concerns would be helpful.
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java (lines 724 - 726)
<https://reviews.apache.org/r/56540/#comment236934>
Use logging here, not System.out.println(...)
ambari-server/src/main/python/ambari_server_main.py (line 247)
<https://reviews.apache.org/r/56540/#comment236935>
+1 for Jonathan's comments. String comparison is evil.
- Nate Cole
On Feb. 10, 2017, 7:21 a.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 7:21 a.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
> On Feb. 10, 2017, 1:38 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java, line 714
> > <https://reviews.apache.org/r/56540/diff/1/?file=1629358#file1629358line714>
> >
> > Doc.
Method is deleted.
- Bal�zs Bence
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165118
-----------------------------------------------------------
On Feb. 14, 2017, 3:27 p.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 3:27 p.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckResult.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyChecker.java 2aaaadd
> ambari-server/src/main/java/org/apache/ambari/server/checks/IncompatibleSchemaException.java PRE-CREATION
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Jonathan Hurley <jh...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165118
-----------------------------------------------------------
Fix it, then Ship it!
ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java (line 714)
<https://reviews.apache.org/r/56540/#comment236931>
Doc.
ambari-server/src/main/python/ambari_server_main.py (line 247)
<https://reviews.apache.org/r/56540/#comment236932>
Could we make this a marker that's more "enum"-like ... Changing a string in Java by accident would cause side effects here that nobody would catch.
- Jonathan Hurley
On Feb. 10, 2017, 7:21 a.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 7:21 a.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Balázs Bence Sári <bs...@hortonworks.com>.
> On Feb. 10, 2017, 12:54 p.m., Sebastian Toader wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java, line 689
> > <https://reviews.apache.org/r/56540/diff/1/?file=1629357#file1629357line689>
> >
> > It would be useful to log the schema name that is first on the search path as well.
The whole search path is logged out, so it is easy to see which is the first item. E.g:
2017-02-09 22:46:25,936 ERROR [main] checks.DatabaseConsistencyCheckHelper (DatabaseConsistencyCheckHelper.java:checkSchemaName(658)) - The schema [ambari] defined for Ambari in ambari.properties is not first on the search path: [public, ambari]. This can lead to problems.
- Bal�zs Bence
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165115
-----------------------------------------------------------
On Feb. 10, 2017, 12:21 p.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 12:21 p.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>
Re: Review Request 56540: Implement new DB checks for Postgres to
prevent cross-schema confusion
Posted by Sebastian Toader <st...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56540/#review165115
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java (line 657)
<https://reviews.apache.org/r/56540/#comment236927>
It would be useful to log the schema name that is first on the search path as well.
- Sebastian Toader
On Feb. 10, 2017, 1:21 p.m., Bal�zs Bence S�ri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56540/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 1:21 p.m.)
>
>
> Review request for Ambari, Attila Doroszlai, Attila Magyar, Jonathan Hurley, Laszlo Puskas, Oliver Szabo, Sandor Magyari, and Sebastian Toader.
>
>
> Bugs: AMBARI-19957
> https://issues.apache.org/jira/browse/AMBARI-19957
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Postgres allows multiple schemas on a database user's search path, that is users can query from tables in different schemas without the need of prefixing the tables in the query.
>
> This can lead to confusion when after an unsuccessful upgrade DBA's restore the tables into a different schema (e.g. public) to Ambari's configured one. As a result, Ambari server may see different data than indended.
>
> New consistency checks on server startup warn the user in such situations.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelper.java 7aa8652
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 1704546
> ambari-server/src/main/python/ambari_server_main.py 7a21333
> ambari-server/src/test/java/org/apache/ambari/server/checks/DatabaseConsistencyCheckHelperTest.java f73562d
>
> Diff: https://reviews.apache.org/r/56540/diff/
>
>
> Testing
> -------
>
> - Wrote new unit tests
> - Run all tests for ambari-server (all passed)
> - Performed manual testing
>
>
> Thanks,
>
> Bal�zs Bence S�ri
>
>