Also SQLException is a checked exception meaning that the CS code knows
how to handle it:
} catch (SQLException ex) {
// if network_groups table exists, create the
default security group there
+ s_logger.debug("Caught SQLException: no
network_group ", ex);
If CS code prefers to ignore it - there is no action in the catch() clause
- then logging the stack trace is not correct. We should either:
a) log it as a one liner error indicating what went wrong if you can
recover from the situation. Just like its done in ResourceManagerImpl by
the same commit for NumberFormatException:
long dcId = -1;
DataCenterVO dc = _dcDao.findByName(dataCenter);
if (dc == null) {
try {
dcId = Long.parseLong(dataCenter);
dc = _dcDao.findById(dcId);
} catch (final NumberFormatException e) {
s_logger.debug("Cannot parse " + dataCenter + " into
Long.”); // logging that was added
}
}
if (dc == null) {
throw new IllegalArgumentException("Host " +
startup.getPrivateIpAddress() + " sent incorrect data center: " +
dataCenter);
}
Or
B) throw CloudStackRuntimeException because inability to save the
system/admin user should block the MS startup as initial login won’t be
possible without the default user saved.
I think b) should be preferable. But b) should come with the changes for
the install script for RPM build.
In any case, fixes done to ConfigurationManagerImpl are not correct, and
logging should be fixed by reverting/reapplying the commit by following
the rules defined in a) or b).
Thank you,
Alena.
On 7/2/14, 2:03 PM, "Daan Hoogland" <daan.hoogl...@gmail.com> wrote:
>I have no idea, the scripts in the scripts/installer dir where written
>by Manuel Amador before moving to apache.
>
>On Wed, Jul 2, 2014 at 10:54 PM, Alena Prokharchyk
><alena.prokharc...@citrix.com> wrote:
>> Its ok to log as long as the original problem - with the install script
>>on
>> RPM setup - is fixed along. I would prefer it to be fixed in a single
>> commit, otherwise the QA will continue seeing this bug and bringing it
>> over and over again. The fix for logging shouldn¹t go to the 4.5 release
>> w/o the original problem fixed, as in this case customers will see the
>> exceptions as well.
>>
>> That¹s why I said ³temporarily².
>>
>> Daan, do you know who might be familiar with installation scripts area?
>>If
>> so, could you CC this person to bring the bug to their attention?
>>
>> Thank you,
>> Alena.
>>
>> On 7/2/14, 1:46 PM, "Daan Hoogland" <daan.hoogl...@gmail.com> wrote:
>>
>>>On Wed, Jul 2, 2014 at 10:29 PM, Alena Prokharchyk
>>><alena.prokharc...@citrix.com> wrote:
>>>> c031eb7d38200d680da85ef57367b21df3483c41
>>>
>>>
>>>So please amend and not revert? I suppose you are talking about
>>>@@ -508,8 +512,9 @@ public class ConfigurationServerImpl extends
>>>ManagerBase implements Configuratio
>>> PreparedStatement stmt =
>>>txn.prepareAutoCloseStatement(checkSql);
>>> stmt.executeQuery();
>>> tableName = "network_group";
>>>- } catch (Exception ex) {
>>>+ } catch (SQLException ex) {
>>> // if network_groups table exists, create the
>>>default security group there
>>>+ s_logger.debug("Caught SQLException: no
>>>network_group ", ex);
>>> }
>>>
>>> insertSql = "SELECT * FROM " + tableName + "
>>>where account_id=2 and name='default'";
>>>
>>>Let's catch and log a generic exception. This is independent of
>>>changing the script. All those other changes are improvements and in
>>>fact this one is as well, no one would have looked at this hidden
>>>feature of the dev env without it. Ignoring exceptions is bad practice
>>>precisely for this reason.
>>>
>>>--
>>>Daan
>>
>
>
>
>--
>Daan
