Ah, you're right. The public update method performs the validation,
then calls the private helper method. I like your additions to the
public JDoc and private comments.
I guess I must've been reading the wrong line of code before, because I
thought modifyDefaultCollectionIntervalForMeasurementDefinitions was
public, at which point the validation should have been there since it
could have been called directly.
In any event, now that I've looked more carefully at the code this time,
I've noticed something different. There are 2 private methods named
modifyDefaultCollectionIntervalForMeasurementDefinitions: one does the
heavy-lifting and database work, and the other "batches" the ids into
groups of 1000 and calls into the heavy-lifter (to avoid
java.sql.SQLException: ORA-01795: "maximum number of expressions in a
list is 1000").
As you can see from the call hierarchy (see attached), not all of the
methods call into the one that does the batching of ids. This means
that users would be able to disable and modify monitoring defaults
against large inventories, but the enable action could fail with the
above oracle error.
On 11/03/2010 03:55 PM, Ian Springer wrote:
I reviews all the related methods again, and the logic looks correct
to
me. I added some additional comments and javadoc to clarify the
semantics of the collection interval parameter.
On 11/02/2010 09:59 AM, Ian Springer wrote:
> Sure, will look into those changes. Thanks for reviewing.
>
> On 11/02/2010 12:56 AM, Joseph Marques wrote:
>> Ian, interesting fix.
>>
>> Looks like the enhancement for
>> 'modifyDefaultCollectionIntervalForMeasurementDefinitions(4 args)' to
>> support the amended JDoc was relatively straightforward. However, I
>> think the logic:
>>
>> - boolean enableDisable = (collectionInterval> 0);
>> + boolean enable = (collectionInterval>= 0);
>>
>> Should move into
>> 'modifyDefaultCollectionIntervalForMeasurementDefinitions(5 args)' which
>> implements the actual update logic. This will allow existing and future
>> call paths to have the same post-condition.
>>
>> Looking deeper into
>> 'modifyDefaultCollectionIntervalForMeasurementDefinitions(5 args)', I
>> notice that we don't do any validation on the 'collectionInterval'
>> parameter. When updating measurement schedules we make sure to call the
>> 'verifyMinimumCollectionInterval' method, but we don't seem to
prevent
>> users from setting default collection intervals to invalid values.
>>
>> Can you add that validation?
>>
>> On 10/29/2010 07:44 PM, ips wrote:
>> ...
>>>
modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/measurement/MeasurementScheduleManagerBean.java
| 15 +-
>>> 7 files changed, 143 insertions(+), 36 deletions(-)
>>>
>>> New commits:
>>> commit 0a0aa07d0e4cdf71b1dee4a7631bd70e50715072
>>> Author: Ian Springer<ian.springer(a)redhat.com>
>>> Date: Fri Oct 29 19:43:38 2010 -0400
>>>
>>> add support to metric template view for specifying whether existing
schedules should be updated
>> ...
>>> diff --git
a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/measurement/MeasurementScheduleManagerBean.java
b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/measurement/MeasurementScheduleManagerBean.java
>>> index ccd65bc..3dc8b05 100644
>>> ---
a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/measurement/MeasurementScheduleManagerBean.java
>>> +++
b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/measurement/MeasurementScheduleManagerBean.java
>>> @@ -332,13 +332,16 @@ public class MeasurementScheduleManagerBean implements
MeasurementScheduleManage
>>> }
>> ...
>>> + * @param collectionInterval if> 0, enable the metric with
this value as the the new collection
>>> + * interval, in milliseconds; if == 0,
enable the metric with its current
>>> + * collection interval; if< 0,
disable the metric
>>> * @param updateExistingSchedules If true, then existing schedules
for this definition will also be updated.
>>> */
>>> @RequiredPermission(Permission.MANAGE_SETTINGS)
>>> @@ -358,13 +361,13 @@ public class MeasurementScheduleManagerBean implements
MeasurementScheduleManage
>>> return;
>>> }
>>>
>>> - boolean enableDisable = (collectionInterval> 0);
>>> + boolean enable = (collectionInterval>= 0);
>>>
>>> // batch the modifications to prevent the ORA error about IN
clauses containing more than 1000 items
>>> for (int batchIndex = 0; (batchIndex<
measurementDefinitionIds.length); batchIndex += 1000) {
>>> int[] batchIdArray =
ArrayUtils.copyOfRange(measurementDefinitionIds, batchIndex, batchIndex + 1000);
>>>
>>> -
modifyDefaultCollectionIntervalForMeasurementDefinitions(subject, batchIdArray,
enableDisable,
>>> +
modifyDefaultCollectionIntervalForMeasurementDefinitions(subject, batchIdArray, enable,
>>> collectionInterval, updateExistingSchedules);
>>> }
>>> }
>>>
>>>
>>> _______________________________________________
>>> rhq-commits mailing list
>>> rhq-commits(a)lists.fedorahosted.org
>>>
https://fedorahosted.org/mailman/listinfo/rhq-commits
>> _______________________________________________
>> rhq-devel mailing list
>> rhq-devel(a)lists.fedorahosted.org
>>
https://fedorahosted.org/mailman/listinfo/rhq-devel