You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Saksham Srivastava <sa...@citrix.com> on 2012/07/16 07:53:34 UTC
Re: Review Request: CS-12707: UI/API - templates> select view: ISOs filter
by: featured or mine> provides unending pages of 2 ISO entries,
DB contains only 19 ISO entries.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5930/
-----------------------------------------------------------
(Updated July 16, 2012, 5:53 a.m.)
Review request for cloudstack and Alena Prokharchyk.
Description
-------
Changes done: Added the restriction on listing ISOs by not allowing total no. of ISOs on a page to exceed the page size.
Made a check on the total public ISOs on a page not to repeat infinetly.
This addresses bug CS-12707.
Diffs
-----
server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 1db12aa
Diff: https://reviews.apache.org/r/5930/diff/
Testing
-------
Verified the fix from both UI and API calls.UI does not show repeating ISOs and API does not list any ISOs for a invalid page request-parameter.
Thanks,
Saksham Srivastava
Re: Review Request: CS-12707: UI/API - templates> select view: ISOs filter
by: featured or mine> provides unending pages of 2 ISO entries,
DB contains only 19 ISO entries.
Posted by Alena Prokharchyk <al...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5930/#review9204
-----------------------------------------------------------
Read it as a continuation for the previous comment.
As:
int i=0;
if (!(startIndex <=isoCount) {
i=(int) (startIndex - isoCount);
}
And make sure you unittest the fix for multiple page situation
- Alena Prokharchyk
On July 16, 2012, 5:53 a.m., Saksham Srivastava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5930/
> -----------------------------------------------------------
>
> (Updated July 16, 2012, 5:53 a.m.)
>
>
> Review request for cloudstack and Alena Prokharchyk.
>
>
> Description
> -------
>
> Changes done: Added the restriction on listing ISOs by not allowing total no. of ISOs on a page to exceed the page size.
> Made a check on the total public ISOs on a page not to repeat infinetly.
>
>
> This addresses bug CS-12707.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 1db12aa
>
> Diff: https://reviews.apache.org/r/5930/diff/
>
>
> Testing
> -------
>
> Verified the fix from both UI and API calls.UI does not show repeating ISOs and API does not list any ISOs for a invalid page request-parameter.
>
>
> Thanks,
>
> Saksham Srivastava
>
>
Re: Review Request: CS-12707: UI/API - templates> select view: ISOs filter
by: featured or mine> provides unending pages of 2 ISO entries,
DB contains only 19 ISO entries.
Posted by Alena Prokharchyk <al...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5930/#review9202
-----------------------------------------------------------
1) Always use Dao methods instead of raw PreparedStatement:
private final String COUNT_ISO ="SELECT COUNT(*) FROM vm_template t INNER JOIN template_zone_ref tzr on (t.id = tzr.template_id) WHERE t.format = 'ISO' AND tzr.removed is null AND t.removed IS NULL ";
pstmt = txn.prepareStatement(COUNT_ISO);
Prepared statements are used only when the query can't be constructed using Dao method, and this is def not the case. Please re-write.
2) Rewrite this piece of code
int i=0;
if (startIndex <=isoCount) {
i=0;
}else {
i=(int) (startIndex - isoCount);
}
- Alena Prokharchyk
On July 16, 2012, 5:53 a.m., Saksham Srivastava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5930/
> -----------------------------------------------------------
>
> (Updated July 16, 2012, 5:53 a.m.)
>
>
> Review request for cloudstack and Alena Prokharchyk.
>
>
> Description
> -------
>
> Changes done: Added the restriction on listing ISOs by not allowing total no. of ISOs on a page to exceed the page size.
> Made a check on the total public ISOs on a page not to repeat infinetly.
>
>
> This addresses bug CS-12707.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 1db12aa
>
> Diff: https://reviews.apache.org/r/5930/diff/
>
>
> Testing
> -------
>
> Verified the fix from both UI and API calls.UI does not show repeating ISOs and API does not list any ISOs for a invalid page request-parameter.
>
>
> Thanks,
>
> Saksham Srivastava
>
>
Re: Review Request: CS-12707: UI/API - templates> select view: ISOs filter
by: featured or mine> provides unending pages of 2 ISO entries,
DB contains only 19 ISO entries.
Posted by Alena Prokharchyk <al...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5930/#review9369
-----------------------------------------------------------
Replace this hardcoded value. Take Iso from Storage.java, ImageFormat enum.
sc.setParameters("format", "ISO");
- Alena Prokharchyk
On July 19, 2012, 10:01 a.m., Saksham Srivastava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5930/
> -----------------------------------------------------------
>
> (Updated July 19, 2012, 10:01 a.m.)
>
>
> Review request for cloudstack and Alena Prokharchyk.
>
>
> Description
> -------
>
> Changes done: Added the restriction on listing ISOs by not allowing total no. of ISOs on a page to exceed the page size.
> Made a check on the total public ISOs on a page not to repeat infinetly.
>
>
> This addresses bug CS-12707.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/dao/VMTemplateDao.java ed80aae
> server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 1db12aa
>
> Diff: https://reviews.apache.org/r/5930/diff/
>
>
> Testing
> -------
>
> Verified the fix from both UI and API calls.UI does not show repeating ISOs and API does not list any ISOs for a invalid page request-parameter.
>
>
> Thanks,
>
> Saksham Srivastava
>
>
Re: Review Request: CS-12707: UI/API - templates> select view: ISOs filter
by: featured or mine> provides unending pages of 2 ISO entries,
DB contains only 19 ISO entries.
Posted by Alena Prokharchyk <al...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5930/#review9414
-----------------------------------------------------------
Ship it!
Ship It!
- Alena Prokharchyk
On July 24, 2012, 5:28 p.m., Saksham Srivastava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5930/
> -----------------------------------------------------------
>
> (Updated July 24, 2012, 5:28 p.m.)
>
>
> Review request for cloudstack and Alena Prokharchyk.
>
>
> Description
> -------
>
> Changes done: Added the restriction on listing ISOs by not allowing total no. of ISOs on a page to exceed the page size.
> Made a check on the total public ISOs on a page not to repeat infinetly.
>
>
> This addresses bug CS-12707.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/dao/VMTemplateDao.java ed80aae
> server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 1db12aa
>
> Diff: https://reviews.apache.org/r/5930/diff/
>
>
> Testing
> -------
>
> Verified the fix from both UI and API calls.UI does not show repeating ISOs and API does not list any ISOs for a invalid page request-parameter.
>
>
> Thanks,
>
> Saksham Srivastava
>
>
Re: Review Request: CS-12707: UI/API - templates> select view: ISOs filter
by: featured or mine> provides unending pages of 2 ISO entries,
DB contains only 19 ISO entries.
Posted by Pranav Saxena <pr...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5930/#review9461
-----------------------------------------------------------
Merged with ASF/master on behalf of Saksham .( Reviewed by Alena)
- Pranav Saxena
On July 24, 2012, 5:28 p.m., Saksham Srivastava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5930/
> -----------------------------------------------------------
>
> (Updated July 24, 2012, 5:28 p.m.)
>
>
> Review request for cloudstack and Alena Prokharchyk.
>
>
> Description
> -------
>
> Changes done: Added the restriction on listing ISOs by not allowing total no. of ISOs on a page to exceed the page size.
> Made a check on the total public ISOs on a page not to repeat infinetly.
>
>
> This addresses bug CS-12707.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/dao/VMTemplateDao.java ed80aae
> server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 1db12aa
>
> Diff: https://reviews.apache.org/r/5930/diff/
>
>
> Testing
> -------
>
> Verified the fix from both UI and API calls.UI does not show repeating ISOs and API does not list any ISOs for a invalid page request-parameter.
>
>
> Thanks,
>
> Saksham Srivastava
>
>
Re: Review Request: CS-12707: UI/API - templates> select view: ISOs filter
by: featured or mine> provides unending pages of 2 ISO entries,
DB contains only 19 ISO entries.
Posted by Saksham Srivastava <sa...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5930/
-----------------------------------------------------------
(Updated July 24, 2012, 5:28 p.m.)
Review request for cloudstack and Alena Prokharchyk.
Changes
-------
Removed hard coded search parameter for ISOs.
Description
-------
Changes done: Added the restriction on listing ISOs by not allowing total no. of ISOs on a page to exceed the page size.
Made a check on the total public ISOs on a page not to repeat infinetly.
This addresses bug CS-12707.
Diffs (updated)
-----
server/src/com/cloud/storage/dao/VMTemplateDao.java ed80aae
server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 1db12aa
Diff: https://reviews.apache.org/r/5930/diff/
Testing
-------
Verified the fix from both UI and API calls.UI does not show repeating ISOs and API does not list any ISOs for a invalid page request-parameter.
Thanks,
Saksham Srivastava
Re: Review Request: CS-12707: UI/API - templates> select view: ISOs filter
by: featured or mine> provides unending pages of 2 ISO entries,
DB contains only 19 ISO entries.
Posted by Saksham Srivastava <sa...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5930/
-----------------------------------------------------------
(Updated July 19, 2012, 10:01 a.m.)
Review request for cloudstack and Alena Prokharchyk.
Changes
-------
All changes made according to review comments.
Replaced prepared statement by DAO implementation for searching non-system ISOs.
Description
-------
Changes done: Added the restriction on listing ISOs by not allowing total no. of ISOs on a page to exceed the page size.
Made a check on the total public ISOs on a page not to repeat infinetly.
This addresses bug CS-12707.
Diffs (updated)
-----
server/src/com/cloud/storage/dao/VMTemplateDao.java ed80aae
server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 1db12aa
Diff: https://reviews.apache.org/r/5930/diff/
Testing
-------
Verified the fix from both UI and API calls.UI does not show repeating ISOs and API does not list any ISOs for a invalid page request-parameter.
Thanks,
Saksham Srivastava
Re: Review Request: CS-12707: UI/API - templates> select view: ISOs filter
by: featured or mine> provides unending pages of 2 ISO entries,
DB contains only 19 ISO entries.
Posted by Alena Prokharchyk <al...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5930/#review9203
-----------------------------------------------------------
1) Always use Dao methods instead of raw PreparedStatement:
private final String COUNT_ISO ="SELECT COUNT(*) FROM vm_template t INNER JOIN template_zone_ref tzr on (t.id = tzr.template_id) WHERE t.format = 'ISO' AND tzr.removed is null AND t.removed IS NULL ";
pstmt = txn.prepareStatement(COUNT_ISO);
Prepared statements are used only when the query can't be constructed using Dao method, and this is def not the case. Please re-write.
2) Rewrite this piece of code
int i=0;
if (startIndex <=isoCount) {
i=0;
}else {
i=(int) (startIndex - isoCount);
}
- Alena Prokharchyk
On July 16, 2012, 5:53 a.m., Saksham Srivastava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5930/
> -----------------------------------------------------------
>
> (Updated July 16, 2012, 5:53 a.m.)
>
>
> Review request for cloudstack and Alena Prokharchyk.
>
>
> Description
> -------
>
> Changes done: Added the restriction on listing ISOs by not allowing total no. of ISOs on a page to exceed the page size.
> Made a check on the total public ISOs on a page not to repeat infinetly.
>
>
> This addresses bug CS-12707.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 1db12aa
>
> Diff: https://reviews.apache.org/r/5930/diff/
>
>
> Testing
> -------
>
> Verified the fix from both UI and API calls.UI does not show repeating ISOs and API does not list any ISOs for a invalid page request-parameter.
>
>
> Thanks,
>
> Saksham Srivastava
>
>