GutoVeronezi commented on code in PR #8230:
URL: https://github.com/apache/cloudstack/pull/8230#discussion_r1501584864
##########
framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java:
##########
@@ -112,24 +111,16 @@ public boolean configure(String name, Map<String, Object>
params) throws Configu
mergeConfigs(configs, params);
}
- String aggregationRange =
configs.get("usage.stats.job.aggregation.range");
- String timeZoneStr = configs.get("usage.aggregation.timezone");
-
- if (timeZoneStr == null) {
- timeZoneStr = "GMT";
- }
- _usageTimezone = TimeZone.getTimeZone(timeZoneStr);
-
- _aggregationDuration = Integer.parseInt(aggregationRange);
- if (_aggregationDuration < UsageUtils.USAGE_AGGREGATION_RANGE_MIN) {
- logger.warn("Usage stats job aggregation range is to small, using
the minimum value of " + UsageUtils.USAGE_AGGREGATION_RANGE_MIN);
- _aggregationDuration = UsageUtils.USAGE_AGGREGATION_RANGE_MIN;
- }
- logger.info("Usage timezone = " + _usageTimezone + "
AggregationDuration=" + _aggregationDuration);
+ String aggregationTimeZoneStr =
ObjectUtils.defaultIfNull(configs.get("usage.aggregation.timezone"), "GMT");
+ usageAggregationTimeZone =
TimeZone.getTimeZone(aggregationTimeZoneStr);
Review Comment:
```suggestion
String usageAggregationTimeZoneStr =
ObjectUtils.defaultIfNull(configs.get("usage.aggregation.timezone"), "GMT");
usageAggregationTimeZone =
TimeZone.getTimeZone(usageAggregationTimeZoneStr);
```
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java:
##########
@@ -470,12 +474,16 @@ protected void
validateEndDateOnCreatingNewQuotaTariff(QuotaTariffVO newQuotaTar
}
if (endDate.compareTo(startDate) < 0) {
- throw new InvalidParameterValueException(String.format("The quota
tariff's end date [%s] cannot be less than the start date [%s]", endDate,
startDate));
+ throw new InvalidParameterValueException(String.format("The quota
tariff's end date [%s] cannot be less than the start date [%s].",
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
endDate),
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
startDate)));
Review Comment:
Same here regarding the value passed as parameter.
##########
framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java:
##########
@@ -366,9 +361,8 @@ protected List<QuotaUsageVO>
persistUsagesAndQuotaUsagesAndRetrievePersistedQuot
protected BigDecimal aggregateQuotaTariffsValues(UsageVO usageRecord,
List<QuotaTariffVO> quotaTariffs, boolean hasAnyQuotaTariffWithActivationRule,
JsInterpreter jsInterpreter, String accountToString) {
- String usageRecordToString = usageRecord.toString();
- logger.debug(String.format("Validating usage record [%s] for account
[%s] against [%s] quota tariffs.", usageRecordToString, accountToString,
- quotaTariffs.size()));
+ String usageRecordToString =
usageRecord.toString(usageAggregationTimeZone);
+ logger.debug("Validating usage record [{}] for {} against [{}] quota
tariffs.", usageRecordToString, accountToString, quotaTariffs.size());
Review Comment:
```suggestion
logger.debug("Validating usage record [{}] for account [{}] against
[{}] quota tariffs.", usageRecordToString, accountToString,
quotaTariffs.size());
```
##########
framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java:
##########
@@ -306,15 +301,15 @@ protected List<UsageVO>
getPendingUsageRecordsForQuotaAggregation(AccountVO acco
protected List<QuotaUsageVO>
createQuotaUsagesAccordingToQuotaTariffs(AccountVO account, List<UsageVO>
usageRecords,
Map<Integer, Pair<List<QuotaTariffVO>, Boolean>>
mapQuotaTariffsPerUsageType) {
String accountToString = account.reflectionToString();
- logger.info(String.format("Calculating quota usage of [%s] usage
records for account [%s].", usageRecords.size(), accountToString));
+ logger.info("Calculating quota usage of [{}] usage records for {}.",
usageRecords.size(), accountToString);
Review Comment:
```suggestion
logger.info("Calculating quota usage of [{}] usage records for
account [{}].", usageRecords.size(), accountToString);
```
`reflectionToString()` does not return the suffix account.
We could use the `toString` as well.
##########
framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java:
##########
@@ -468,10 +462,11 @@ protected boolean
isQuotaTariffInPeriodToBeApplied(UsageVO usageRecord, QuotaTar
Date quotaTariffEndDate = quotaTariff.getEndDate();
if ((quotaTariffEndDate != null &&
usageRecordStartDate.after(quotaTariffEndDate)) ||
usageRecordEndDate.before(quotaTariffStartDate)) {
- logger.debug(String.format("Not applying quota tariff [%s] in
usage record [%s] of account [%s] due to it is out of the period to be applied.
Period of the usage"
- + " record [startDate: %s, endDate: %s], period of the
quota tariff [startDate: %s, endDate: %s].", quotaTariff,
usageRecord.toString(), accountToString,
- DateUtil.getOutputString(usageRecordStartDate),
DateUtil.getOutputString(usageRecordEndDate),
DateUtil.getOutputString(quotaTariffStartDate),
- DateUtil.getOutputString(quotaTariffEndDate)));
+ logger.debug("Not applying quota tariff [{}] in usage record [{}]
of {} due to it is out of the period to be applied. Period of the usage"
Review Comment:
```suggestion
logger.debug("Not applying quota tariff [{}] in usage record
[{}] of account [{}] due to it is out of the period to be applied. Period of
the usage"
```
##########
framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java:
##########
@@ -497,11 +492,11 @@ protected Map<Integer, Pair<List<QuotaTariffVO>,
Boolean>> createMapQuotaTariffs
}
protected QuotaUsageVO createQuotaUsageAccordingToUsageUnit(UsageVO
usageRecord, BigDecimal aggregatedQuotaTariffsValue, String accountToString) {
- String usageRecordToString = usageRecord.toString();
+ String usageRecordToString =
usageRecord.toString(usageAggregationTimeZone);
if (aggregatedQuotaTariffsValue.equals(BigDecimal.ZERO)) {
- logger.debug(String.format("Usage record [%s] for account [%s]
does not have quota tariffs to be calculated, therefore we will mark it as
calculated.",
- usageRecordToString, accountToString));
+ logger.debug("No tariffs were applied to usage record [{}] of {}
or they resulted in 0; We will only mark the usage record as calculated.",
Review Comment:
```suggestion
logger.debug("No tariffs were applied to usage record [{}] of
account [{}] or they resulted in 0; We will only mark the usage record as
calculated.",
```
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java:
##########
@@ -470,12 +474,16 @@ protected void
validateEndDateOnCreatingNewQuotaTariff(QuotaTariffVO newQuotaTar
}
if (endDate.compareTo(startDate) < 0) {
- throw new InvalidParameterValueException(String.format("The quota
tariff's end date [%s] cannot be less than the start date [%s]", endDate,
startDate));
+ throw new InvalidParameterValueException(String.format("The quota
tariff's end date [%s] cannot be less than the start date [%s].",
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
endDate),
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
startDate)));
}
Date now = _quotaService.computeAdjustedTime(new Date());
if (endDate.compareTo(now) < 0) {
- throw new InvalidParameterValueException(String.format("The quota
tariff's end date [%s] cannot be less than now [%s].", endDate, now));
+ throw new InvalidParameterValueException(String.format("The quota
tariff's end date [%s] cannot be less than now [%s].",
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
endDate),
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
now)));
Review Comment:
Same here regarding the value passed as parameter.
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java:
##########
@@ -374,8 +376,10 @@ public Pair<List<QuotaTariffVO>, Integer>
listQuotaTariffPlans(final QuotaTariff
Long startIndex = cmd.getStartIndex();
Long pageSize = cmd.getPageSizeVal();
- logger.debug(String.format("Listing quota tariffs for parameters
[%s].", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(cmd,
"effectiveDate",
- "endDate", "listAll", "name", "page", "pageSize",
"usageType")));
+ logger.debug("Listing quota tariffs for parameters [{}] between [{}]
and [{}].", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(cmd,
"listAll",
+ "name", "page", "pageSize", "usageType"),
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
startDate),
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
endDate));
Review Comment:
As these are values passed as parameter, it does not make sense to display
them in the configured timezone; they should be printed as they were passed
(the raw value).
##########
usage/src/main/java/com/cloud/usage/UsageManagerImpl.java:
##########
@@ -254,25 +254,29 @@ public boolean configure(String name, Map<String, Object>
params) throws Configu
}
int hourOfDay = Integer.parseInt(execTimeSegments[0]);
int minutes = Integer.parseInt(execTimeSegments[1]);
- _jobExecTime.setTime(new Date());
+
+ Date currentDate = new Date();
+ _jobExecTime.setTime(currentDate);
_jobExecTime.set(Calendar.HOUR_OF_DAY, hourOfDay);
_jobExecTime.set(Calendar.MINUTE, minutes);
_jobExecTime.set(Calendar.SECOND, 0);
_jobExecTime.set(Calendar.MILLISECOND, 0);
- if (execTimeZone != null && !execTimeZone.isEmpty()) {
- _jobExecTime.setTimeZone(TimeZone.getTimeZone(execTimeZone));
- }
+
+ TimeZone jobExecTimeZone = execTimeZone != null ?
TimeZone.getTimeZone(execTimeZone) : Calendar.getInstance().getTimeZone();
+ _jobExecTime.setTimeZone(jobExecTimeZone);
// if the hour to execute the job has already passed, roll the day
forward to the next day
- Date execDate = _jobExecTime.getTime();
- if (execDate.before(new Date())) {
+ if (_jobExecTime.getTime().before(currentDate)) {
_jobExecTime.roll(Calendar.DAY_OF_YEAR, true);
}
- logger.debug("Execution Time: " + execDate.toString());
- Date currentDate = new Date(System.currentTimeMillis());
- logger.debug("Current Time: " + currentDate.toString());
+ logger.info("Usage is configured to execute in time zone [{}], at
[{}], each [{}] minutes; the current time in that timezone is [{}] and the " +
+ "next job is scheduled to execute at [{}]. During
its execution, Usage will aggregate stats according to the boundaries of a day
in time zone [{}].",
Review Comment:
```suggestion
"next job is scheduled to execute at [{}].
During its execution, Usage will aggregate stats according to the time zone
[{}] defined in global setting [{}]...
```
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java:
##########
@@ -487,7 +495,8 @@ public QuotaCreditsResponse addQuotaCredits(Long accountId,
Long domainId, Doubl
QuotaBalanceVO qb = _quotaBalanceDao.findLaterBalanceEntry(accountId,
domainId, despositedOn);
if (qb != null) {
- throw new InvalidParameterValueException("Incorrect deposit date:
" + despositedOn + " there are balance entries after this date");
+ throw new InvalidParameterValueException(String.format("Incorrect
deposit date [%s], as there are balance entries after this date.",
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
despositedOn)));
Review Comment:
Same here regarding the value passed as parameter.
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java:
##########
@@ -638,7 +647,9 @@ public QuotaTariffVO createQuotaTariff(QuotaTariffCreateCmd
cmd) {
}
if (startDate.compareTo(now) < 0) {
- throw new InvalidParameterValueException(String.format("The quota
tariff's start date [%s] cannot be less than now [%s]", startDate, now));
+ throw new InvalidParameterValueException(String.format("The quota
tariff's start date [%s] cannot be less than now [%s].",
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
startDate),
+
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
now)));
Review Comment:
Same here regarding the value passed as parameter.
```suggestion
throw new InvalidParameterValueException(String.format("The
value passed as Quota tariff's start date is in the past: [%s]. Please, inform
a date in the future or do not pass the parameter to use the current date and
time.",
DateUtil.displayDateInTimezone(QuotaManagerImpl.getUsageAggregationTimeZone(),
startDate)));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
