> On July 3, 2013, 3:58 p.m., bharat kumar wrote:
> > server/src/com/cloud/configuration/ConfigurationManagerImpl.java, line 3054
> > <https://reviews.apache.org/r/12251/diff/1/?file=318002#file318002line3054>
> >
> >     removed the sql exception
> 
> Alena Prokharchyk wrote:
>     Bharat, I still see SQLException being caught:
>     
>     } catch (SQLException e) {
>     3090      
>                 throw new CloudRuntimeException(e.getMessage());
>     3104      
>             }
>     3091      
>             }
>     
>     Can you please remove this try/catch as you are not throwing the 
> exception any longer?
> 
> bharat kumar wrote:
>     The function deletePublicIPRangeExceptAliasIP used in 
> handleIpAliasDeletion throws the sql exception.

Well, you should remove it. This code:

    @Override
    public boolean deletePublicIPRangeExceptAliasIP(long vlanDbId, String 
aliasIp) throws SQLException {
        Transaction txn = Transaction.currentTxn();
        String deleteSql = "DELETE FROM `cloud`.`user_ip_address` WHERE 
vlan_db_id = ? and public_ip_address!=?";

        txn.start();
        PreparedStatement stmt = txn.prepareAutoCloseStatement(deleteSql);
        stmt.setLong(1, vlanDbId);
        stmt.setString(2, aliasIp);
        stmt.executeUpdate();
        txn.commit();
        return true;
    }

uses raw mysql update. We should never do that. What if 
vlan_db_id/public_ip_address are renamed or gone in the future? It won't be 
caught during compilation time. Please rewrite this code the following way:

* use SearchBuilder or SearchCriteria to identify the rows you need to remove. 
Remove the raw mysql statement.
* remove PreparedStatement update.
* remove SQLException from the throws clause and from the underlying try/catch 
blocks.


- Alena


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


On July 4, 2013, 2:09 a.m., bharat kumar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
> 
> (Updated July 4, 2013, 2:09 a.m.)
> 
> 
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and 
> Sheng Yang.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Incorporating the review comments given by Alena and Sheng.
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/network/Network.java a06208b 
>   api/src/com/cloud/network/NetworkService.java 405cecd 
>   server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51 
>   server/src/com/cloud/network/NetworkManagerImpl.java 708c03d 
>   server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde 
>   server/src/com/cloud/network/element/VirtualRouterElement.java 1916678 
>   server/test/com/cloud/network/MockNetworkManagerImpl.java 077395f 
>   server/test/com/cloud/vpc/MockNetworkManagerImpl.java b609022 
> 
> Diff: https://reviews.apache.org/r/12251/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> bharat kumar
> 
>

Reply via email to