You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Ajit Kumar <aj...@hortonworks.com> on 2016/02/04 01:36:14 UTC
Review Request 42838: Add upgrade support for Setting feature
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/
-----------------------------------------------------------
Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
Bugs: AMBARI-14912
https://issues.apache.org/jira/browse/AMBARI-14912
Repository: ambari
Description
-------
Update upgrade catalog 240 and add db schema changes for settings table.
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
Diff: https://reviews.apache.org/r/42838/diff/
Testing
-------
Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
Thanks,
Ajit Kumar
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Ajit Kumar <aj...@hortonworks.com>.
> On Feb. 9, 2016, 8:40 p.m., Sumit Mohanty wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java, line 70
> > <https://reviews.apache.org/r/42838/diff/1/?file=1231923#file1231923line70>
> >
> > Nit: spelling for AUTHORIZATION
Fixed.
- Ajit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118461
-----------------------------------------------------------
On Feb. 9, 2016, 9:23 p.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 9:23 p.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Sumit Mohanty <sm...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118461
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java (line 70)
<https://reviews.apache.org/r/42838/#comment179691>
Nit: spelling for AUTHORIZATION
- Sumit Mohanty
On Feb. 4, 2016, 12:36 a.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:36 a.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Sid Wagle <sw...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review119385
-----------------------------------------------------------
Ship it!
General comment: It is cleaner to work with entities than issuing direct SQL.
- Sid Wagle
On Feb. 16, 2016, 11:36 p.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2016, 11:36 p.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> ran ambari-server upgrade maually and checked impacted db tables to make sure correct entry is present.
>
>
> After successfully upgrade, updated metainfo table with old version and ran ambari-server upgrade and it was successful.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Ajit Kumar <aj...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/
-----------------------------------------------------------
(Updated Feb. 16, 2016, 11:36 p.m.)
Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
Changes
-------
Added checks to avoid duplicate inserts.
Bugs: AMBARI-14912
https://issues.apache.org/jira/browse/AMBARI-14912
Repository: ambari
Description
-------
Update upgrade catalog 240 and add db schema changes for settings table.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
Diff: https://reviews.apache.org/r/42838/diff/
Testing (updated)
-------
ran ambari-server upgrade maually and checked impacted db tables to make sure correct entry is present.
After successfully upgrade, updated metainfo table with old version and ran ambari-server upgrade and it was successful.
Thanks,
Ajit Kumar
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Sumit Mohanty <sm...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118718
-----------------------------------------------------------
Ship it!
Ship It!
- Sumit Mohanty
On Feb. 9, 2016, 9:23 p.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 9:23 p.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Ajit Kumar <aj...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/
-----------------------------------------------------------
(Updated Feb. 9, 2016, 9:23 p.m.)
Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
Changes
-------
Fixing spelling for SETTINGS_AUTHORIZATION.
Bugs: AMBARI-14912
https://issues.apache.org/jira/browse/AMBARI-14912
Repository: ambari
Description
-------
Update upgrade catalog 240 and add db schema changes for settings table.
Diffs (updated)
-----
ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
Diff: https://reviews.apache.org/r/42838/diff/
Testing
-------
Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
Thanks,
Ajit Kumar
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Jayush Luniya <jl...@hortonworks.com>.
> On Feb. 9, 2016, 6:40 p.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java, line 130
> > <https://reviews.apache.org/r/42838/diff/1/?file=1231923#file1231923line130>
> >
> > This should be idempotent. If anything fails in this file, we should be able to retry.
> > Please add a check that the table doesn't already exist.
> > Before inserting those 2 rows, can check if they already exist.
>
> Nahappan Somasundaram wrote:
> SchemaUpgradeHelper::main() or at least SchemaUpgradeHelper::executeUpgrade() should be transactional. If anything fails, we should be able to leave the DB in the state it was found, instead of a partially upgraded schema, which will break the system if the upgrade is not run again. It is cumbersome to verify each upgrade for partial execution before re-running the upgrade.
>
> If an upgrade was successful, re-running the upgrade should exit with a message. I suppose this is already happening.
>
> Ajit Kumar wrote:
> I agree with Nahappan here. Ideal way to deal with the problem is make sure db changes are transactional. I looked at other UpgradeCatalog*.java and there is no validation during table creation if table already exists. If current implementation of UpgradeCatalog can leave DB in inconsistent/partial upgrade state, we should handle that as a separate task.
>
> Alejandro Fernandez wrote:
> The changes are not transactional. This is just good practice, check if the table and records exist before attempting to insert them. Also, if another unrelated change fails as part of the upgrade and this change for Settings was already done and the upgrade must be re-ran, we don't want it to fail. This is a very critical part of Ambari.
>
> Sid Wagle wrote:
> All DDL methods on DBAccessorImpl do a check before create, so the createTable, createFK etc are all idempotent.
> Although we haven't made everything transactional, idempotency is a must, only is upgrade completes successfully we do not re-run the last Catalog.
+1 on making this transactional. However we should evaluate and track this in a separate work item. I agree with @Alejandro that we should add checks before table creation and inserts. Just good defensive programming.
- Jayush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118429
-----------------------------------------------------------
On Feb. 4, 2016, 12:36 a.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:36 a.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Ajit Kumar <aj...@hortonworks.com>.
> On Feb. 9, 2016, 6:40 p.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java, line 130
> > <https://reviews.apache.org/r/42838/diff/1/?file=1231923#file1231923line130>
> >
> > This should be idempotent. If anything fails in this file, we should be able to retry.
> > Please add a check that the table doesn't already exist.
> > Before inserting those 2 rows, can check if they already exist.
>
> Nahappan Somasundaram wrote:
> SchemaUpgradeHelper::main() or at least SchemaUpgradeHelper::executeUpgrade() should be transactional. If anything fails, we should be able to leave the DB in the state it was found, instead of a partially upgraded schema, which will break the system if the upgrade is not run again. It is cumbersome to verify each upgrade for partial execution before re-running the upgrade.
>
> If an upgrade was successful, re-running the upgrade should exit with a message. I suppose this is already happening.
>
> Ajit Kumar wrote:
> I agree with Nahappan here. Ideal way to deal with the problem is make sure db changes are transactional. I looked at other UpgradeCatalog*.java and there is no validation during table creation if table already exists. If current implementation of UpgradeCatalog can leave DB in inconsistent/partial upgrade state, we should handle that as a separate task.
>
> Alejandro Fernandez wrote:
> The changes are not transactional. This is just good practice, check if the table and records exist before attempting to insert them. Also, if another unrelated change fails as part of the upgrade and this change for Settings was already done and the upgrade must be re-ran, we don't want it to fail. This is a very critical part of Ambari.
>
> Sid Wagle wrote:
> All DDL methods on DBAccessorImpl do a check before create, so the createTable, createFK etc are all idempotent.
> Although we haven't made everything transactional, idempotency is a must, only is upgrade completes successfully we do not re-run the last Catalog.
>
> Jayush Luniya wrote:
> +1 on making this transactional. However we should evaluate and track this in a separate work item. I agree with @Alejandro that we should add checks before table creation and inserts. Just good defensive programming.
Looks like we don't need to added checks for table creation. I ran ambari-server upgrade twice and only insert failed for duplicate inserts and table creation did not throw error.
- Ajit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118429
-----------------------------------------------------------
On Feb. 16, 2016, 11:36 p.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2016, 11:36 p.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> ran ambari-server upgrade maually and checked impacted db tables to make sure correct entry is present.
>
>
> After successfully upgrade, updated metainfo table with old version and ran ambari-server upgrade and it was successful.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Ajit Kumar <aj...@hortonworks.com>.
> On Feb. 9, 2016, 6:40 p.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java, line 130
> > <https://reviews.apache.org/r/42838/diff/1/?file=1231923#file1231923line130>
> >
> > This should be idempotent. If anything fails in this file, we should be able to retry.
> > Please add a check that the table doesn't already exist.
> > Before inserting those 2 rows, can check if they already exist.
>
> Nahappan Somasundaram wrote:
> SchemaUpgradeHelper::main() or at least SchemaUpgradeHelper::executeUpgrade() should be transactional. If anything fails, we should be able to leave the DB in the state it was found, instead of a partially upgraded schema, which will break the system if the upgrade is not run again. It is cumbersome to verify each upgrade for partial execution before re-running the upgrade.
>
> If an upgrade was successful, re-running the upgrade should exit with a message. I suppose this is already happening.
I agree with Nahappan here. Ideal way to deal with the problem is make sure db changes are transactional. I looked at other UpgradeCatalog*.java and there is no validation during table creation if table already exists. If current implementation of UpgradeCatalog can leave DB in inconsistent/partial upgrade state, we should handle that as a separate task.
- Ajit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118429
-----------------------------------------------------------
On Feb. 4, 2016, 12:36 a.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:36 a.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Alejandro Fernandez <af...@hortonworks.com>.
> On Feb. 9, 2016, 6:40 p.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java, line 130
> > <https://reviews.apache.org/r/42838/diff/1/?file=1231923#file1231923line130>
> >
> > This should be idempotent. If anything fails in this file, we should be able to retry.
> > Please add a check that the table doesn't already exist.
> > Before inserting those 2 rows, can check if they already exist.
>
> Nahappan Somasundaram wrote:
> SchemaUpgradeHelper::main() or at least SchemaUpgradeHelper::executeUpgrade() should be transactional. If anything fails, we should be able to leave the DB in the state it was found, instead of a partially upgraded schema, which will break the system if the upgrade is not run again. It is cumbersome to verify each upgrade for partial execution before re-running the upgrade.
>
> If an upgrade was successful, re-running the upgrade should exit with a message. I suppose this is already happening.
>
> Ajit Kumar wrote:
> I agree with Nahappan here. Ideal way to deal with the problem is make sure db changes are transactional. I looked at other UpgradeCatalog*.java and there is no validation during table creation if table already exists. If current implementation of UpgradeCatalog can leave DB in inconsistent/partial upgrade state, we should handle that as a separate task.
The changes are not transactional. This is just good practice, check if the table and records exist before attempting to insert them. Also, if another unrelated change fails as part of the upgrade and this change for Settings was already done and the upgrade must be re-ran, we don't want it to fail. This is a very critical part of Ambari.
- Alejandro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118429
-----------------------------------------------------------
On Feb. 4, 2016, 12:36 a.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:36 a.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Sid Wagle <sw...@hortonworks.com>.
> On Feb. 9, 2016, 6:40 p.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java, line 130
> > <https://reviews.apache.org/r/42838/diff/1/?file=1231923#file1231923line130>
> >
> > This should be idempotent. If anything fails in this file, we should be able to retry.
> > Please add a check that the table doesn't already exist.
> > Before inserting those 2 rows, can check if they already exist.
>
> Nahappan Somasundaram wrote:
> SchemaUpgradeHelper::main() or at least SchemaUpgradeHelper::executeUpgrade() should be transactional. If anything fails, we should be able to leave the DB in the state it was found, instead of a partially upgraded schema, which will break the system if the upgrade is not run again. It is cumbersome to verify each upgrade for partial execution before re-running the upgrade.
>
> If an upgrade was successful, re-running the upgrade should exit with a message. I suppose this is already happening.
>
> Ajit Kumar wrote:
> I agree with Nahappan here. Ideal way to deal with the problem is make sure db changes are transactional. I looked at other UpgradeCatalog*.java and there is no validation during table creation if table already exists. If current implementation of UpgradeCatalog can leave DB in inconsistent/partial upgrade state, we should handle that as a separate task.
>
> Alejandro Fernandez wrote:
> The changes are not transactional. This is just good practice, check if the table and records exist before attempting to insert them. Also, if another unrelated change fails as part of the upgrade and this change for Settings was already done and the upgrade must be re-ran, we don't want it to fail. This is a very critical part of Ambari.
All DDL methods on DBAccessorImpl do a check before create, so the createTable, createFK etc are all idempotent.
Although we haven't made everything transactional, idempotency is a must, only is upgrade completes successfully we do not re-run the last Catalog.
- Sid
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118429
-----------------------------------------------------------
On Feb. 4, 2016, 12:36 a.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:36 a.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Nahappan Somasundaram <ns...@hortonworks.com>.
> On Feb. 9, 2016, 10:40 a.m., Alejandro Fernandez wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java, line 130
> > <https://reviews.apache.org/r/42838/diff/1/?file=1231923#file1231923line130>
> >
> > This should be idempotent. If anything fails in this file, we should be able to retry.
> > Please add a check that the table doesn't already exist.
> > Before inserting those 2 rows, can check if they already exist.
SchemaUpgradeHelper::main() or at least SchemaUpgradeHelper::executeUpgrade() should be transactional. If anything fails, we should be able to leave the DB in the state it was found, instead of a partially upgraded schema, which will break the system if the upgrade is not run again. It is cumbersome to verify each upgrade for partial execution before re-running the upgrade.
If an upgrade was successful, re-running the upgrade should exit with a message. I suppose this is already happening.
- Nahappan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118429
-----------------------------------------------------------
On Feb. 3, 2016, 4:36 p.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 4:36 p.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Alejandro Fernandez <af...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118429
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java (line 129)
<https://reviews.apache.org/r/42838/#comment179638>
This should be idempotent. If anything fails in this file, we should be able to retry.
Please add a check that the table doesn't already exist.
Before inserting those 2 rows, can check if they already exist.
- Alejandro Fernandez
On Feb. 4, 2016, 12:36 a.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:36 a.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Nahappan Somasundaram <ns...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118428
-----------------------------------------------------------
Ship it!
Ship It!
- Nahappan Somasundaram
On Feb. 3, 2016, 4:36 p.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 4:36 p.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Ajit Kumar <aj...@hortonworks.com>.
> On Feb. 9, 2016, 7:57 p.m., Sid Wagle wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java, line 146
> > <https://reviews.apache.org/r/42838/diff/1/?file=1231923#file1231923line146>
> >
> > Check before insert.
Added.
- Ajit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118453
-----------------------------------------------------------
On Feb. 16, 2016, 11:36 p.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2016, 11:36 p.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> ran ambari-server upgrade maually and checked impacted db tables to make sure correct entry is present.
>
>
> After successfully upgrade, updated metainfo table with old version and ran ambari-server upgrade and it was successful.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Sid Wagle <sw...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118453
-----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java (line 145)
<https://reviews.apache.org/r/42838/#comment179683>
Check before insert.
- Sid Wagle
On Feb. 4, 2016, 12:36 a.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:36 a.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Nahappan Somasundaram <ns...@hortonworks.com>.
I’m going over your changes. I’ll try it out on my VM to see what’s going on.
From: Ajit Kumar <no...@reviews.apache.org>> on behalf of Ajit Kumar <aj...@hortonworks.com>>
Reply-To: Ajit Kumar <aj...@hortonworks.com>>
Date: Monday, February 8, 2016 at 11:06 AM
To: Jayush Luniya <jl...@hortonworks.com>>, Nahappan Somasundaram <ns...@hortonworks.com>>, Sumit Mohanty <sm...@hortonworks.com>>
Cc: Ajit Kumar <aj...@hortonworks.com>>, Ambari <de...@ambari.apache.org>>
Subject: Re: Review Request 42838: Add upgrade support for Setting feature
This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42838/
I tried to upgrade from 2.2.1 and it is failing while running UpgradeCatalog230.
Exception in thread "main" org.apache.ambari.server.AmbariException: ERROR: insert or update on table "permission_roleauthorization" violates foreign key constraint "fk_permission_roleauth_aid"
Detail: Key (authorization_id)=(CLUSTER.MANAGE_CONFIG_GROUPS) is not present in table "roleauthorization".
at org.apache.ambari.server.upgrade.SchemaUpgradeHelper.executeDMLUpdates(SchemaUpgradeHelper.java:233)
at org.apache.ambari.server.upgrade.SchemaUpgradeHelper.main(SchemaUpgradeHelper.java:307)
Caused by: org.postgresql.util.PSQLException: ERROR: insert or update on table "permission_roleauthorization" violates foreign key constraint "fk_permission_roleauth_aid"
Detail: Key (authorization_id)=(CLUSTER.MANAGE_CONFIG_GROUPS) is not present in table "roleauthorization".
at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2161)
at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:1890)
at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:255)
at org.postgresql.jdbc2.AbstractJdbc2Statement.execute(AbstractJdbc2Statement.java:559)
at org.postgresql.jdbc2.AbstractJdbc2Statement.executeWithFlags(AbstractJdbc2Statement.java:403)
at org.postgresql.jdbc2.AbstractJdbc2Statement.executeUpdate(AbstractJdbc2Statement.java:331)
at org.apache.ambari.server.orm.DBAccessorImpl.insertRow(DBAccessorImpl.java:635)
at org.apache.ambari.server.upgrade.UpgradeCatalog230.createPermissionRoleAuthorizationMap(UpgradeCatalog230.java:317)
at org.apache.ambari.server.upgrade.UpgradeCatalog230.executeDMLUpdates(UpgradeCatalog230.java:129)
at org.apache.ambari.server.upgrade.AbstractUpgradeCatalog.upgradeData(AbstractUpgradeCatalog.java:659)
at org.apache.ambari.server.upgrade.SchemaUpgradeHelper.executeDMLUpdates(SchemaUpgradeHelper.java:230)
... 1 more
- Ajit Kumar
On February 4th, 2016, 12:36 a.m. UTC, Ajit Kumar wrote:
Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
By Ajit Kumar.
Updated Feb. 4, 2016, 12:36 a.m.
Bugs: AMBARI-14912<https://issues.apache.org/jira/browse/AMBARI-14912>
Repository: ambari
Description
Update upgrade catalog 240 and add db schema changes for settings table.
Testing
Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
Diffs
* ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java (3414388b9a738a4fc7cc5d0f484fe553cce840ee)
* ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java (d1d68f24871422936321ad0c940499fc1ef4cbec)
View Diff<https://reviews.apache.org/r/42838/diff/>
Re: Review Request 42838: Add upgrade support for Setting feature
Posted by Ajit Kumar <aj...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42838/#review118273
-----------------------------------------------------------
I tried to upgrade from 2.2.1 and it is failing while running UpgradeCatalog230.
Exception in thread "main" org.apache.ambari.server.AmbariException: ERROR: insert or update on table "permission_roleauthorization" violates foreign key constraint "fk_permission_roleauth_aid"
Detail: Key (authorization_id)=(CLUSTER.MANAGE_CONFIG_GROUPS) is not present in table "roleauthorization".
at org.apache.ambari.server.upgrade.SchemaUpgradeHelper.executeDMLUpdates(SchemaUpgradeHelper.java:233)
at org.apache.ambari.server.upgrade.SchemaUpgradeHelper.main(SchemaUpgradeHelper.java:307)
Caused by: org.postgresql.util.PSQLException: ERROR: insert or update on table "permission_roleauthorization" violates foreign key constraint "fk_permission_roleauth_aid"
Detail: Key (authorization_id)=(CLUSTER.MANAGE_CONFIG_GROUPS) is not present in table "roleauthorization".
at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2161)
at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:1890)
at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:255)
at org.postgresql.jdbc2.AbstractJdbc2Statement.execute(AbstractJdbc2Statement.java:559)
at org.postgresql.jdbc2.AbstractJdbc2Statement.executeWithFlags(AbstractJdbc2Statement.java:403)
at org.postgresql.jdbc2.AbstractJdbc2Statement.executeUpdate(AbstractJdbc2Statement.java:331)
at org.apache.ambari.server.orm.DBAccessorImpl.insertRow(DBAccessorImpl.java:635)
at org.apache.ambari.server.upgrade.UpgradeCatalog230.createPermissionRoleAuthorizationMap(UpgradeCatalog230.java:317)
at org.apache.ambari.server.upgrade.UpgradeCatalog230.executeDMLUpdates(UpgradeCatalog230.java:129)
at org.apache.ambari.server.upgrade.AbstractUpgradeCatalog.upgradeData(AbstractUpgradeCatalog.java:659)
at org.apache.ambari.server.upgrade.SchemaUpgradeHelper.executeDMLUpdates(SchemaUpgradeHelper.java:230)
... 1 more
- Ajit Kumar
On Feb. 4, 2016, 12:36 a.m., Ajit Kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42838/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 12:36 a.m.)
>
>
> Review request for Ambari, Jayush Luniya, Nahappan Somasundaram, and Sumit Mohanty.
>
>
> Bugs: AMBARI-14912
> https://issues.apache.org/jira/browse/AMBARI-14912
>
>
> Repository: ambari
>
>
> Description
> -------
>
> Update upgrade catalog 240 and add db schema changes for settings table.
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog240.java 3414388b9a738a4fc7cc5d0f484fe553cce840ee
> ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog240Test.java d1d68f24871422936321ad0c940499fc1ef4cbec
>
> Diff: https://reviews.apache.org/r/42838/diff/
>
>
> Testing
> -------
>
> Unit testing. Currently my local trunk build is breaking even w/o my changes. I'll run mvn clean install when it is fixed.
>
>
> Thanks,
>
> Ajit Kumar
>
>