Copilot commented on code in PR #11443:
URL: https://github.com/apache/cloudstack/pull/11443#discussion_r3153325352
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -1225,57 +1224,64 @@ protected void checkPortParameters(final String
publicPort, final String private
}
@Override
- public boolean archiveEvents(final ArchiveEventsCmd cmd) {
- final Account caller = getCaller();
- final List<Long> ids = cmd.getIds();
- boolean result = true;
- List<Long> permittedAccountIds = new ArrayList<>();
+ public boolean archiveEvents(ArchiveEventsCmd cmd) {
+ List<Long> ids = cmd.getIds();
+ String type = cmd.getType();
+ Date startDate = cmd.getStartDate();
+ Date endDate = cmd.getEndDate();
+
+ validateEventOperationParameters(ids, type, endDate, startDate);
+
+ Long accountId = null;
+ List<Long> domainIds = null;
+ Account caller = getCaller();
+ Account.Type callerType = caller.getType();
- if (_accountService.isNormalUser(caller.getId()) || caller.getType()
== Account.Type.PROJECT) {
- permittedAccountIds.add(caller.getId());
+ if (callerType == Account.Type.NORMAL || callerType ==
Account.Type.PROJECT) {
+ accountId = caller.getId();
} else {
- final DomainVO domain = _domainDao.findById(caller.getDomainId());
- final List<Long> permittedDomainIds =
_domainDao.getDomainChildrenIds(domain.getPath());
- permittedAccountIds =
_accountDao.getAccountIdsForDomains(permittedDomainIds);
+ domainIds =
_domainDao.getDomainAndChildrenIds(caller.getDomainId());
}
- final List<EventVO> events =
_eventDao.listToArchiveOrDeleteEvents(ids, cmd.getType(), cmd.getStartDate(),
cmd.getEndDate(), permittedAccountIds);
- final ControlledEntity[] sameOwnerEvents = events.toArray(new
ControlledEntity[events.size()]);
- _accountMgr.checkAccess(CallContext.current().getCallingAccount(),
null, false, sameOwnerEvents);
+ long totalArchived = _eventDao.archiveEvents(ids, type, startDate,
endDate, accountId, domainIds,
+ ConfigurationManagerImpl.DELETE_QUERY_BATCH_SIZE.value());
- if (ids != null && events.size() < ids.size()) {
- return false;
- }
- _eventDao.archiveEvents(events);
- return result;
+ return totalArchived > 0;
Review Comment:
When `ids` is provided, this method reports success if *any* matching rows
were archived. That means a request like `ids=[1,2,3]` can return success even
if only a subset was actually archived (e.g., some IDs don’t exist or aren’t in
the caller’s scope). If the API is expected to be all-or-nothing for explicit
IDs, consider returning failure (or throwing) when `totalArchived !=
ids.size()` when `ids` is non-empty, or otherwise make the partial-success
behavior explicit.
##########
engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java:
##########
@@ -33,83 +38,90 @@
@Component
public class EventDaoImpl extends GenericDaoBase<EventVO, Long> implements
EventDao {
- protected final SearchBuilder<EventVO> CompletedEventSearch;
+
protected final SearchBuilder<EventVO> ToArchiveOrDeleteEventSearch;
public EventDaoImpl() {
- CompletedEventSearch = createSearchBuilder();
- CompletedEventSearch.and("state",
CompletedEventSearch.entity().getState(), SearchCriteria.Op.EQ);
- CompletedEventSearch.and("startId",
CompletedEventSearch.entity().getStartId(), SearchCriteria.Op.EQ);
- CompletedEventSearch.and("archived",
CompletedEventSearch.entity().getArchived(), Op.EQ);
- CompletedEventSearch.done();
-
ToArchiveOrDeleteEventSearch = createSearchBuilder();
+ ToArchiveOrDeleteEventSearch.select("id", SearchCriteria.Func.NATIVE,
ToArchiveOrDeleteEventSearch.entity().getId());
ToArchiveOrDeleteEventSearch.and("id",
ToArchiveOrDeleteEventSearch.entity().getId(), Op.IN);
ToArchiveOrDeleteEventSearch.and("type",
ToArchiveOrDeleteEventSearch.entity().getType(), Op.EQ);
- ToArchiveOrDeleteEventSearch.and("accountIds",
ToArchiveOrDeleteEventSearch.entity().getAccountId(), Op.IN);
+ ToArchiveOrDeleteEventSearch.and("accountId",
ToArchiveOrDeleteEventSearch.entity().getAccountId(), Op.EQ);
+ ToArchiveOrDeleteEventSearch.and("domainIds",
ToArchiveOrDeleteEventSearch.entity().getDomainId(), Op.IN);
ToArchiveOrDeleteEventSearch.and("createdDateB",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.BETWEEN);
ToArchiveOrDeleteEventSearch.and("createdDateL",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.LTEQ);
+ ToArchiveOrDeleteEventSearch.and("createdDateLT",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.LT);
ToArchiveOrDeleteEventSearch.and("archived",
ToArchiveOrDeleteEventSearch.entity().getArchived(), Op.EQ);
ToArchiveOrDeleteEventSearch.done();
}
- @Override
- public List<EventVO> searchAllEvents(SearchCriteria<EventVO> sc, Filter
filter) {
- return listIncludingRemovedBy(sc, filter);
- }
-
- @Override
- public List<EventVO> listOlderEvents(Date oldTime) {
- if (oldTime == null)
- return null;
- SearchCriteria<EventVO> sc = createSearchCriteria();
- sc.addAnd("createDate", SearchCriteria.Op.LT, oldTime);
- sc.addAnd("archived", SearchCriteria.Op.EQ, false);
- return listIncludingRemovedBy(sc, null);
- }
-
- @Override
- public EventVO findCompletedEvent(long startId) {
- SearchCriteria<EventVO> sc = CompletedEventSearch.create();
- sc.setParameters("state", State.Completed);
- sc.setParameters("startId", startId);
- sc.setParameters("archived", false);
- return findOneIncludingRemovedBy(sc);
- }
-
- @Override
- public List<EventVO> listToArchiveOrDeleteEvents(List<Long> ids, String
type, Date startDate, Date endDate, List<Long> accountIds) {
+ private SearchCriteria<EventVO> createEventSearchCriteria(List<Long> ids,
String type, Date startDate, Date endDate,
+ Date limitDate,
Long accountId, List<Long> domainIds) {
SearchCriteria<EventVO> sc = ToArchiveOrDeleteEventSearch.create();
- if (ids != null) {
- sc.setParameters("id", ids.toArray(new Object[ids.size()]));
+
+ if (CollectionUtils.isNotEmpty(ids)) {
+ sc.setParameters("id", ids.toArray(new Object[0]));
}
- if (type != null) {
- sc.setParameters("type", type);
+ if (CollectionUtils.isNotEmpty(domainIds)) {
+ sc.setParameters("domainIds", domainIds.toArray(new Object[0]));
}
if (startDate != null && endDate != null) {
sc.setParameters("createdDateB", startDate, endDate);
} else if (endDate != null) {
sc.setParameters("createdDateL", endDate);
}
- if (accountIds != null && !accountIds.isEmpty()) {
- sc.setParameters("accountIds", accountIds.toArray(new
Object[accountIds.size()]));
- }
+ sc.setParametersIfNotNull("accountId", accountId);
+ sc.setParametersIfNotNull("createdDateLT", limitDate);
+ sc.setParametersIfNotNull("type", type);
sc.setParameters("archived", false);
- return search(sc, null);
+
+ return sc;
}
@Override
- public void archiveEvents(List<EventVO> events) {
- if (events != null && !events.isEmpty()) {
- TransactionLegacy txn = TransactionLegacy.currentTxn();
- txn.start();
- for (EventVO event : events) {
- event = lockRow(event.getId(), true);
- event.setArchived(true);
- update(event.getId(), event);
- txn.commit();
+ public long archiveEvents(List<Long> ids, String type, Date startDate,
Date endDate, Long accountId, List<Long> domainIds,
+ long limitPerQuery) {
+ SearchCriteria<EventVO> sc = createEventSearchCriteria(ids, type,
startDate, endDate, null, accountId, domainIds);
+ Filter filter = null;
+ if (limitPerQuery > 0) {
+ filter = new Filter(limitPerQuery);
+ }
+
+ long archived;
+ long totalArchived = 0L;
+
+ do {
+ List<EventVO> events = search(sc, filter);
+ if (events.isEmpty()) {
+ break;
}
- txn.close();
+
+ archived = archiveEventsInternal(events);
+ totalArchived += archived;
+ } while (limitPerQuery > 0 && archived >= limitPerQuery);
Review Comment:
`archiveEvents` can load *all* matching events into memory when
`limitPerQuery <= 0` (the default for `delete.query.batch.size` is 0), then
builds a single `UPDATE ... WHERE id IN (...)` statement containing every ID.
On large event tables this risks OOMs, oversized SQL statements, and very
long-running queries. Consider always archiving in batches (even when the
config is 0), or implement a batched `UPDATE ... LIMIT <batch>` loop similar to
`batchExpunge` so the operation remains bounded.
##########
api/src/main/java/org/apache/cloudstack/api/command/user/event/ArchiveEventsCmd.java:
##########
@@ -97,17 +112,12 @@ public long getEntityOwnerId() {
@Override
public void execute() {
- if (ids == null && type == null && endDate == null) {
- throw new InvalidParameterValueException("either ids, type or
enddate must be specified");
- } else if (startDate != null && endDate == null) {
- throw new InvalidParameterValueException("enddate must be
specified with startdate parameter");
- }
boolean result = _mgr.archiveEvents(this);
if (result) {
SuccessResponse response = new SuccessResponse(getCommandName());
setResponseObject(response);
- } else {
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Unable
to archive Events, one or more parameters has invalid values");
+ return;
}
+ throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Unable to
archive Events. One or more parameters have invalid values.");
}
Review Comment:
`_mgr.archiveEvents` returns `false` when `totalArchived == 0`, which can
happen with valid parameters that match zero rows. Throwing an INTERNAL_ERROR
that claims invalid parameters is misleading in that case. Consider
distinguishing "no matching events" from actual validation errors (which should
throw `InvalidParameterValueException`) and adjusting the message accordingly.
##########
engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java:
##########
@@ -33,83 +38,90 @@
@Component
public class EventDaoImpl extends GenericDaoBase<EventVO, Long> implements
EventDao {
- protected final SearchBuilder<EventVO> CompletedEventSearch;
+
protected final SearchBuilder<EventVO> ToArchiveOrDeleteEventSearch;
public EventDaoImpl() {
- CompletedEventSearch = createSearchBuilder();
- CompletedEventSearch.and("state",
CompletedEventSearch.entity().getState(), SearchCriteria.Op.EQ);
- CompletedEventSearch.and("startId",
CompletedEventSearch.entity().getStartId(), SearchCriteria.Op.EQ);
- CompletedEventSearch.and("archived",
CompletedEventSearch.entity().getArchived(), Op.EQ);
- CompletedEventSearch.done();
-
ToArchiveOrDeleteEventSearch = createSearchBuilder();
+ ToArchiveOrDeleteEventSearch.select("id", SearchCriteria.Func.NATIVE,
ToArchiveOrDeleteEventSearch.entity().getId());
ToArchiveOrDeleteEventSearch.and("id",
ToArchiveOrDeleteEventSearch.entity().getId(), Op.IN);
ToArchiveOrDeleteEventSearch.and("type",
ToArchiveOrDeleteEventSearch.entity().getType(), Op.EQ);
- ToArchiveOrDeleteEventSearch.and("accountIds",
ToArchiveOrDeleteEventSearch.entity().getAccountId(), Op.IN);
+ ToArchiveOrDeleteEventSearch.and("accountId",
ToArchiveOrDeleteEventSearch.entity().getAccountId(), Op.EQ);
+ ToArchiveOrDeleteEventSearch.and("domainIds",
ToArchiveOrDeleteEventSearch.entity().getDomainId(), Op.IN);
ToArchiveOrDeleteEventSearch.and("createdDateB",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.BETWEEN);
ToArchiveOrDeleteEventSearch.and("createdDateL",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.LTEQ);
+ ToArchiveOrDeleteEventSearch.and("createdDateLT",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.LT);
ToArchiveOrDeleteEventSearch.and("archived",
ToArchiveOrDeleteEventSearch.entity().getArchived(), Op.EQ);
ToArchiveOrDeleteEventSearch.done();
}
- @Override
- public List<EventVO> searchAllEvents(SearchCriteria<EventVO> sc, Filter
filter) {
- return listIncludingRemovedBy(sc, filter);
- }
-
- @Override
- public List<EventVO> listOlderEvents(Date oldTime) {
- if (oldTime == null)
- return null;
- SearchCriteria<EventVO> sc = createSearchCriteria();
- sc.addAnd("createDate", SearchCriteria.Op.LT, oldTime);
- sc.addAnd("archived", SearchCriteria.Op.EQ, false);
- return listIncludingRemovedBy(sc, null);
- }
-
- @Override
- public EventVO findCompletedEvent(long startId) {
- SearchCriteria<EventVO> sc = CompletedEventSearch.create();
- sc.setParameters("state", State.Completed);
- sc.setParameters("startId", startId);
- sc.setParameters("archived", false);
- return findOneIncludingRemovedBy(sc);
- }
-
- @Override
- public List<EventVO> listToArchiveOrDeleteEvents(List<Long> ids, String
type, Date startDate, Date endDate, List<Long> accountIds) {
+ private SearchCriteria<EventVO> createEventSearchCriteria(List<Long> ids,
String type, Date startDate, Date endDate,
+ Date limitDate,
Long accountId, List<Long> domainIds) {
SearchCriteria<EventVO> sc = ToArchiveOrDeleteEventSearch.create();
- if (ids != null) {
- sc.setParameters("id", ids.toArray(new Object[ids.size()]));
+
+ if (CollectionUtils.isNotEmpty(ids)) {
+ sc.setParameters("id", ids.toArray(new Object[0]));
}
- if (type != null) {
- sc.setParameters("type", type);
+ if (CollectionUtils.isNotEmpty(domainIds)) {
+ sc.setParameters("domainIds", domainIds.toArray(new Object[0]));
}
if (startDate != null && endDate != null) {
sc.setParameters("createdDateB", startDate, endDate);
} else if (endDate != null) {
sc.setParameters("createdDateL", endDate);
}
- if (accountIds != null && !accountIds.isEmpty()) {
- sc.setParameters("accountIds", accountIds.toArray(new
Object[accountIds.size()]));
- }
+ sc.setParametersIfNotNull("accountId", accountId);
+ sc.setParametersIfNotNull("createdDateLT", limitDate);
+ sc.setParametersIfNotNull("type", type);
sc.setParameters("archived", false);
- return search(sc, null);
+
+ return sc;
}
@Override
- public void archiveEvents(List<EventVO> events) {
- if (events != null && !events.isEmpty()) {
- TransactionLegacy txn = TransactionLegacy.currentTxn();
- txn.start();
- for (EventVO event : events) {
- event = lockRow(event.getId(), true);
- event.setArchived(true);
- update(event.getId(), event);
- txn.commit();
+ public long archiveEvents(List<Long> ids, String type, Date startDate,
Date endDate, Long accountId, List<Long> domainIds,
+ long limitPerQuery) {
+ SearchCriteria<EventVO> sc = createEventSearchCriteria(ids, type,
startDate, endDate, null, accountId, domainIds);
+ Filter filter = null;
+ if (limitPerQuery > 0) {
+ filter = new Filter(limitPerQuery);
+ }
+
+ long archived;
+ long totalArchived = 0L;
+
+ do {
+ List<EventVO> events = search(sc, filter);
+ if (events.isEmpty()) {
+ break;
}
- txn.close();
+
+ archived = archiveEventsInternal(events);
+ totalArchived += archived;
+ } while (limitPerQuery > 0 && archived >= limitPerQuery);
+
+ return totalArchived;
+ }
+
+ @DB
+ private long archiveEventsInternal(List<EventVO> events) {
+ final String idsAsString = events.stream()
+ .map(e -> Long.toString(e.getId()))
+ .collect(Collectors.joining(","));
+ final String query = String.format("UPDATE event SET archived=true
WHERE id IN (%s)", idsAsString);
+
+ try (TransactionLegacy txn = TransactionLegacy.currentTxn();
+ PreparedStatement pstmt = txn.prepareStatement(query)) {
+ return pstmt.executeUpdate();
+ } catch (SQLException e) {
+ throw new CloudRuntimeException(e);
}
}
+
+ @Override
+ public long purgeAll(List<Long> ids, Date startDate, Date endDate, Date
limitDate, String type, Long accountId,
+ List<Long> domainIds, long limitPerQuery) {
+ SearchCriteria<EventVO> sc = createEventSearchCriteria(ids, type,
startDate, endDate, limitDate, accountId, domainIds);
+ return batchExpunge(sc, limitPerQuery);
+ }
Review Comment:
New batching semantics in `archiveEvents(...)`/`purgeAll(...)` are
non-trivial (limit handling, iteration/termination, criteria composition) but
there are currently no unit tests covering these paths (unlike other DAO
batch-expunge usages in `engine/schema/src/test`). Consider adding a focused
DAO unit test validating that multiple batches are processed correctly and that
`limitPerQuery <= 0` behaves as intended.
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -1225,57 +1224,64 @@ protected void checkPortParameters(final String
publicPort, final String private
}
@Override
- public boolean archiveEvents(final ArchiveEventsCmd cmd) {
- final Account caller = getCaller();
- final List<Long> ids = cmd.getIds();
- boolean result = true;
- List<Long> permittedAccountIds = new ArrayList<>();
+ public boolean archiveEvents(ArchiveEventsCmd cmd) {
+ List<Long> ids = cmd.getIds();
+ String type = cmd.getType();
+ Date startDate = cmd.getStartDate();
+ Date endDate = cmd.getEndDate();
+
+ validateEventOperationParameters(ids, type, endDate, startDate);
+
+ Long accountId = null;
+ List<Long> domainIds = null;
+ Account caller = getCaller();
+ Account.Type callerType = caller.getType();
- if (_accountService.isNormalUser(caller.getId()) || caller.getType()
== Account.Type.PROJECT) {
- permittedAccountIds.add(caller.getId());
+ if (callerType == Account.Type.NORMAL || callerType ==
Account.Type.PROJECT) {
+ accountId = caller.getId();
} else {
- final DomainVO domain = _domainDao.findById(caller.getDomainId());
- final List<Long> permittedDomainIds =
_domainDao.getDomainChildrenIds(domain.getPath());
- permittedAccountIds =
_accountDao.getAccountIdsForDomains(permittedDomainIds);
+ domainIds =
_domainDao.getDomainAndChildrenIds(caller.getDomainId());
}
- final List<EventVO> events =
_eventDao.listToArchiveOrDeleteEvents(ids, cmd.getType(), cmd.getStartDate(),
cmd.getEndDate(), permittedAccountIds);
- final ControlledEntity[] sameOwnerEvents = events.toArray(new
ControlledEntity[events.size()]);
- _accountMgr.checkAccess(CallContext.current().getCallingAccount(),
null, false, sameOwnerEvents);
+ long totalArchived = _eventDao.archiveEvents(ids, type, startDate,
endDate, accountId, domainIds,
+ ConfigurationManagerImpl.DELETE_QUERY_BATCH_SIZE.value());
- if (ids != null && events.size() < ids.size()) {
- return false;
- }
- _eventDao.archiveEvents(events);
- return result;
+ return totalArchived > 0;
}
@Override
- public boolean deleteEvents(final DeleteEventsCmd cmd) {
- final Account caller = getCaller();
- final List<Long> ids = cmd.getIds();
- boolean result = true;
- List<Long> permittedAccountIds = new ArrayList<>();
+ public boolean deleteEvents(DeleteEventsCmd cmd) {
+ List<Long> ids = cmd.getIds();
+ String type = cmd.getType();
+ Date startDate = cmd.getStartDate();
+ Date endDate = cmd.getEndDate();
- if (_accountMgr.isNormalUser(caller.getId()) || caller.getType() ==
Account.Type.PROJECT) {
- permittedAccountIds.add(caller.getId());
+ validateEventOperationParameters(ids, type, endDate, startDate);
+
+ Long accountId = null;
+ List<Long> domainIds = null;
+ Account caller = getCaller();
+ Account.Type callerType = caller.getType();
+
+ if (callerType == Account.Type.NORMAL || callerType ==
Account.Type.PROJECT) {
+ accountId = caller.getId();
} else {
- final DomainVO domain = _domainDao.findById(caller.getDomainId());
- final List<Long> permittedDomainIds =
_domainDao.getDomainChildrenIds(domain.getPath());
- permittedAccountIds =
_accountDao.getAccountIdsForDomains(permittedDomainIds);
+ domainIds =
_domainDao.getDomainAndChildrenIds(caller.getDomainId());
}
- final List<EventVO> events =
_eventDao.listToArchiveOrDeleteEvents(ids, cmd.getType(), cmd.getStartDate(),
cmd.getEndDate(), permittedAccountIds);
- final ControlledEntity[] sameOwnerEvents = events.toArray(new
ControlledEntity[events.size()]);
- _accountMgr.checkAccess(CallContext.current().getCallingAccount(),
null, false, sameOwnerEvents);
+ long totalRemoved = _eventDao.purgeAll(ids, startDate, endDate, null,
type, accountId, domainIds,
+ ConfigurationManagerImpl.DELETE_QUERY_BATCH_SIZE.value());
- if (ids != null && events.size() < ids.size()) {
- return false;
+ return totalRemoved > 0;
+ }
+
+ protected void validateEventOperationParameters(List<Long> ids, String
type, Date endDate, Date startDate) {
+ if (CollectionUtils.isEmpty(ids) && ObjectUtils.allNull(type,
endDate)) {
+ throw new InvalidParameterValueException("Either 'ids', 'type' or
'enddate' must be specified.");
}
- for (final EventVO event : events) {
- _eventDao.remove(event.getId());
+ if (startDate != null && endDate == null) {
+ throw new InvalidParameterValueException("'startdate' must be
specified with 'enddate' parameter.");
Review Comment:
`validateEventOperationParameters` throws when `startDate != null && endDate
== null`, but the exception message says "'startdate' must be specified with
'enddate'", which is the opposite of what’s wrong in this case. Update the
message to indicate that `enddate` is required when `startdate` is provided
(and keep parameter naming consistent with the API).
```suggestion
throw new InvalidParameterValueException("'enddate' must be
specified when 'startdate' parameter is provided.");
```
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -1225,57 +1224,64 @@ protected void checkPortParameters(final String
publicPort, final String private
}
@Override
- public boolean archiveEvents(final ArchiveEventsCmd cmd) {
- final Account caller = getCaller();
- final List<Long> ids = cmd.getIds();
- boolean result = true;
- List<Long> permittedAccountIds = new ArrayList<>();
+ public boolean archiveEvents(ArchiveEventsCmd cmd) {
+ List<Long> ids = cmd.getIds();
+ String type = cmd.getType();
+ Date startDate = cmd.getStartDate();
+ Date endDate = cmd.getEndDate();
+
+ validateEventOperationParameters(ids, type, endDate, startDate);
+
+ Long accountId = null;
+ List<Long> domainIds = null;
+ Account caller = getCaller();
+ Account.Type callerType = caller.getType();
- if (_accountService.isNormalUser(caller.getId()) || caller.getType()
== Account.Type.PROJECT) {
- permittedAccountIds.add(caller.getId());
+ if (callerType == Account.Type.NORMAL || callerType ==
Account.Type.PROJECT) {
+ accountId = caller.getId();
} else {
- final DomainVO domain = _domainDao.findById(caller.getDomainId());
- final List<Long> permittedDomainIds =
_domainDao.getDomainChildrenIds(domain.getPath());
- permittedAccountIds =
_accountDao.getAccountIdsForDomains(permittedDomainIds);
+ domainIds =
_domainDao.getDomainAndChildrenIds(caller.getDomainId());
}
- final List<EventVO> events =
_eventDao.listToArchiveOrDeleteEvents(ids, cmd.getType(), cmd.getStartDate(),
cmd.getEndDate(), permittedAccountIds);
- final ControlledEntity[] sameOwnerEvents = events.toArray(new
ControlledEntity[events.size()]);
- _accountMgr.checkAccess(CallContext.current().getCallingAccount(),
null, false, sameOwnerEvents);
+ long totalArchived = _eventDao.archiveEvents(ids, type, startDate,
endDate, accountId, domainIds,
+ ConfigurationManagerImpl.DELETE_QUERY_BATCH_SIZE.value());
- if (ids != null && events.size() < ids.size()) {
- return false;
- }
- _eventDao.archiveEvents(events);
- return result;
+ return totalArchived > 0;
}
@Override
- public boolean deleteEvents(final DeleteEventsCmd cmd) {
- final Account caller = getCaller();
- final List<Long> ids = cmd.getIds();
- boolean result = true;
- List<Long> permittedAccountIds = new ArrayList<>();
+ public boolean deleteEvents(DeleteEventsCmd cmd) {
+ List<Long> ids = cmd.getIds();
+ String type = cmd.getType();
+ Date startDate = cmd.getStartDate();
+ Date endDate = cmd.getEndDate();
- if (_accountMgr.isNormalUser(caller.getId()) || caller.getType() ==
Account.Type.PROJECT) {
- permittedAccountIds.add(caller.getId());
+ validateEventOperationParameters(ids, type, endDate, startDate);
+
+ Long accountId = null;
+ List<Long> domainIds = null;
+ Account caller = getCaller();
+ Account.Type callerType = caller.getType();
+
+ if (callerType == Account.Type.NORMAL || callerType ==
Account.Type.PROJECT) {
+ accountId = caller.getId();
} else {
- final DomainVO domain = _domainDao.findById(caller.getDomainId());
- final List<Long> permittedDomainIds =
_domainDao.getDomainChildrenIds(domain.getPath());
- permittedAccountIds =
_accountDao.getAccountIdsForDomains(permittedDomainIds);
+ domainIds =
_domainDao.getDomainAndChildrenIds(caller.getDomainId());
}
- final List<EventVO> events =
_eventDao.listToArchiveOrDeleteEvents(ids, cmd.getType(), cmd.getStartDate(),
cmd.getEndDate(), permittedAccountIds);
- final ControlledEntity[] sameOwnerEvents = events.toArray(new
ControlledEntity[events.size()]);
- _accountMgr.checkAccess(CallContext.current().getCallingAccount(),
null, false, sameOwnerEvents);
+ long totalRemoved = _eventDao.purgeAll(ids, startDate, endDate, null,
type, accountId, domainIds,
+ ConfigurationManagerImpl.DELETE_QUERY_BATCH_SIZE.value());
Review Comment:
When `ids` is provided, this method reports success if *any* rows were
deleted. This can silently ignore IDs that don’t match the criteria / caller
scope while still returning success, which is potentially misleading for
callers expecting all requested IDs to be removed. Consider aligning the return
semantics with the requested operation (e.g., fail when `ids` is non-empty and
`totalRemoved != ids.size()`, or explicitly document/return partial results).
```suggestion
if (CollectionUtils.isNotEmpty(ids)) {
return totalRemoved == ids.size();
}
```
##########
engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java:
##########
@@ -33,83 +38,90 @@
@Component
public class EventDaoImpl extends GenericDaoBase<EventVO, Long> implements
EventDao {
- protected final SearchBuilder<EventVO> CompletedEventSearch;
+
protected final SearchBuilder<EventVO> ToArchiveOrDeleteEventSearch;
public EventDaoImpl() {
- CompletedEventSearch = createSearchBuilder();
- CompletedEventSearch.and("state",
CompletedEventSearch.entity().getState(), SearchCriteria.Op.EQ);
- CompletedEventSearch.and("startId",
CompletedEventSearch.entity().getStartId(), SearchCriteria.Op.EQ);
- CompletedEventSearch.and("archived",
CompletedEventSearch.entity().getArchived(), Op.EQ);
- CompletedEventSearch.done();
-
ToArchiveOrDeleteEventSearch = createSearchBuilder();
+ ToArchiveOrDeleteEventSearch.select("id", SearchCriteria.Func.NATIVE,
ToArchiveOrDeleteEventSearch.entity().getId());
ToArchiveOrDeleteEventSearch.and("id",
ToArchiveOrDeleteEventSearch.entity().getId(), Op.IN);
ToArchiveOrDeleteEventSearch.and("type",
ToArchiveOrDeleteEventSearch.entity().getType(), Op.EQ);
- ToArchiveOrDeleteEventSearch.and("accountIds",
ToArchiveOrDeleteEventSearch.entity().getAccountId(), Op.IN);
+ ToArchiveOrDeleteEventSearch.and("accountId",
ToArchiveOrDeleteEventSearch.entity().getAccountId(), Op.EQ);
+ ToArchiveOrDeleteEventSearch.and("domainIds",
ToArchiveOrDeleteEventSearch.entity().getDomainId(), Op.IN);
ToArchiveOrDeleteEventSearch.and("createdDateB",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.BETWEEN);
ToArchiveOrDeleteEventSearch.and("createdDateL",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.LTEQ);
+ ToArchiveOrDeleteEventSearch.and("createdDateLT",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.LT);
ToArchiveOrDeleteEventSearch.and("archived",
ToArchiveOrDeleteEventSearch.entity().getArchived(), Op.EQ);
ToArchiveOrDeleteEventSearch.done();
}
- @Override
- public List<EventVO> searchAllEvents(SearchCriteria<EventVO> sc, Filter
filter) {
- return listIncludingRemovedBy(sc, filter);
- }
-
- @Override
- public List<EventVO> listOlderEvents(Date oldTime) {
- if (oldTime == null)
- return null;
- SearchCriteria<EventVO> sc = createSearchCriteria();
- sc.addAnd("createDate", SearchCriteria.Op.LT, oldTime);
- sc.addAnd("archived", SearchCriteria.Op.EQ, false);
- return listIncludingRemovedBy(sc, null);
- }
-
- @Override
- public EventVO findCompletedEvent(long startId) {
- SearchCriteria<EventVO> sc = CompletedEventSearch.create();
- sc.setParameters("state", State.Completed);
- sc.setParameters("startId", startId);
- sc.setParameters("archived", false);
- return findOneIncludingRemovedBy(sc);
- }
-
- @Override
- public List<EventVO> listToArchiveOrDeleteEvents(List<Long> ids, String
type, Date startDate, Date endDate, List<Long> accountIds) {
+ private SearchCriteria<EventVO> createEventSearchCriteria(List<Long> ids,
String type, Date startDate, Date endDate,
+ Date limitDate,
Long accountId, List<Long> domainIds) {
SearchCriteria<EventVO> sc = ToArchiveOrDeleteEventSearch.create();
- if (ids != null) {
- sc.setParameters("id", ids.toArray(new Object[ids.size()]));
+
+ if (CollectionUtils.isNotEmpty(ids)) {
+ sc.setParameters("id", ids.toArray(new Object[0]));
}
- if (type != null) {
- sc.setParameters("type", type);
+ if (CollectionUtils.isNotEmpty(domainIds)) {
+ sc.setParameters("domainIds", domainIds.toArray(new Object[0]));
}
if (startDate != null && endDate != null) {
sc.setParameters("createdDateB", startDate, endDate);
} else if (endDate != null) {
sc.setParameters("createdDateL", endDate);
}
- if (accountIds != null && !accountIds.isEmpty()) {
- sc.setParameters("accountIds", accountIds.toArray(new
Object[accountIds.size()]));
- }
+ sc.setParametersIfNotNull("accountId", accountId);
+ sc.setParametersIfNotNull("createdDateLT", limitDate);
+ sc.setParametersIfNotNull("type", type);
sc.setParameters("archived", false);
- return search(sc, null);
+
+ return sc;
}
@Override
- public void archiveEvents(List<EventVO> events) {
- if (events != null && !events.isEmpty()) {
- TransactionLegacy txn = TransactionLegacy.currentTxn();
- txn.start();
- for (EventVO event : events) {
- event = lockRow(event.getId(), true);
- event.setArchived(true);
- update(event.getId(), event);
- txn.commit();
+ public long archiveEvents(List<Long> ids, String type, Date startDate,
Date endDate, Long accountId, List<Long> domainIds,
+ long limitPerQuery) {
+ SearchCriteria<EventVO> sc = createEventSearchCriteria(ids, type,
startDate, endDate, null, accountId, domainIds);
+ Filter filter = null;
+ if (limitPerQuery > 0) {
+ filter = new Filter(limitPerQuery);
+ }
+
+ long archived;
+ long totalArchived = 0L;
+
+ do {
+ List<EventVO> events = search(sc, filter);
+ if (events.isEmpty()) {
+ break;
}
- txn.close();
+
+ archived = archiveEventsInternal(events);
+ totalArchived += archived;
+ } while (limitPerQuery > 0 && archived >= limitPerQuery);
+
+ return totalArchived;
+ }
+
+ @DB
+ private long archiveEventsInternal(List<EventVO> events) {
+ final String idsAsString = events.stream()
+ .map(e -> Long.toString(e.getId()))
+ .collect(Collectors.joining(","));
+ final String query = String.format("UPDATE event SET archived=true
WHERE id IN (%s)", idsAsString);
+
+ try (TransactionLegacy txn = TransactionLegacy.currentTxn();
+ PreparedStatement pstmt = txn.prepareStatement(query)) {
Review Comment:
`archiveEventsInternal` builds SQL by string-concatenating the IDs into an
`IN (...)` clause. Besides the query-size risk, it bypasses parameter binding
and can stress SQL parsing/plan caching. Consider using a parameterized
statement (placeholders) or a different batching strategy (e.g., update by
criteria with `LIMIT`) to keep statements small and reusable.
```suggestion
if (CollectionUtils.isEmpty(events)) {
return 0L;
}
final String placeholders = events.stream()
.map(event -> "?")
.collect(Collectors.joining(","));
final String query = String.format("UPDATE event SET archived=true
WHERE id IN (%s)", placeholders);
try (TransactionLegacy txn = TransactionLegacy.currentTxn();
PreparedStatement pstmt = txn.prepareStatement(query)) {
for (int i = 0; i < events.size(); i++) {
pstmt.setLong(i + 1, events.get(i).getId());
}
```
##########
engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java:
##########
@@ -33,83 +38,90 @@
@Component
public class EventDaoImpl extends GenericDaoBase<EventVO, Long> implements
EventDao {
- protected final SearchBuilder<EventVO> CompletedEventSearch;
+
protected final SearchBuilder<EventVO> ToArchiveOrDeleteEventSearch;
public EventDaoImpl() {
- CompletedEventSearch = createSearchBuilder();
- CompletedEventSearch.and("state",
CompletedEventSearch.entity().getState(), SearchCriteria.Op.EQ);
- CompletedEventSearch.and("startId",
CompletedEventSearch.entity().getStartId(), SearchCriteria.Op.EQ);
- CompletedEventSearch.and("archived",
CompletedEventSearch.entity().getArchived(), Op.EQ);
- CompletedEventSearch.done();
-
ToArchiveOrDeleteEventSearch = createSearchBuilder();
+ ToArchiveOrDeleteEventSearch.select("id", SearchCriteria.Func.NATIVE,
ToArchiveOrDeleteEventSearch.entity().getId());
ToArchiveOrDeleteEventSearch.and("id",
ToArchiveOrDeleteEventSearch.entity().getId(), Op.IN);
ToArchiveOrDeleteEventSearch.and("type",
ToArchiveOrDeleteEventSearch.entity().getType(), Op.EQ);
- ToArchiveOrDeleteEventSearch.and("accountIds",
ToArchiveOrDeleteEventSearch.entity().getAccountId(), Op.IN);
+ ToArchiveOrDeleteEventSearch.and("accountId",
ToArchiveOrDeleteEventSearch.entity().getAccountId(), Op.EQ);
+ ToArchiveOrDeleteEventSearch.and("domainIds",
ToArchiveOrDeleteEventSearch.entity().getDomainId(), Op.IN);
ToArchiveOrDeleteEventSearch.and("createdDateB",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.BETWEEN);
ToArchiveOrDeleteEventSearch.and("createdDateL",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.LTEQ);
+ ToArchiveOrDeleteEventSearch.and("createdDateLT",
ToArchiveOrDeleteEventSearch.entity().getCreateDate(), Op.LT);
ToArchiveOrDeleteEventSearch.and("archived",
ToArchiveOrDeleteEventSearch.entity().getArchived(), Op.EQ);
ToArchiveOrDeleteEventSearch.done();
}
- @Override
- public List<EventVO> searchAllEvents(SearchCriteria<EventVO> sc, Filter
filter) {
- return listIncludingRemovedBy(sc, filter);
- }
-
- @Override
- public List<EventVO> listOlderEvents(Date oldTime) {
- if (oldTime == null)
- return null;
- SearchCriteria<EventVO> sc = createSearchCriteria();
- sc.addAnd("createDate", SearchCriteria.Op.LT, oldTime);
- sc.addAnd("archived", SearchCriteria.Op.EQ, false);
- return listIncludingRemovedBy(sc, null);
- }
-
- @Override
- public EventVO findCompletedEvent(long startId) {
- SearchCriteria<EventVO> sc = CompletedEventSearch.create();
- sc.setParameters("state", State.Completed);
- sc.setParameters("startId", startId);
- sc.setParameters("archived", false);
- return findOneIncludingRemovedBy(sc);
- }
-
- @Override
- public List<EventVO> listToArchiveOrDeleteEvents(List<Long> ids, String
type, Date startDate, Date endDate, List<Long> accountIds) {
+ private SearchCriteria<EventVO> createEventSearchCriteria(List<Long> ids,
String type, Date startDate, Date endDate,
+ Date limitDate,
Long accountId, List<Long> domainIds) {
SearchCriteria<EventVO> sc = ToArchiveOrDeleteEventSearch.create();
- if (ids != null) {
- sc.setParameters("id", ids.toArray(new Object[ids.size()]));
+
+ if (CollectionUtils.isNotEmpty(ids)) {
+ sc.setParameters("id", ids.toArray(new Object[0]));
}
- if (type != null) {
- sc.setParameters("type", type);
+ if (CollectionUtils.isNotEmpty(domainIds)) {
+ sc.setParameters("domainIds", domainIds.toArray(new Object[0]));
}
if (startDate != null && endDate != null) {
sc.setParameters("createdDateB", startDate, endDate);
} else if (endDate != null) {
sc.setParameters("createdDateL", endDate);
}
- if (accountIds != null && !accountIds.isEmpty()) {
- sc.setParameters("accountIds", accountIds.toArray(new
Object[accountIds.size()]));
- }
+ sc.setParametersIfNotNull("accountId", accountId);
+ sc.setParametersIfNotNull("createdDateLT", limitDate);
+ sc.setParametersIfNotNull("type", type);
sc.setParameters("archived", false);
- return search(sc, null);
+
+ return sc;
}
@Override
- public void archiveEvents(List<EventVO> events) {
- if (events != null && !events.isEmpty()) {
- TransactionLegacy txn = TransactionLegacy.currentTxn();
- txn.start();
- for (EventVO event : events) {
- event = lockRow(event.getId(), true);
- event.setArchived(true);
- update(event.getId(), event);
- txn.commit();
+ public long archiveEvents(List<Long> ids, String type, Date startDate,
Date endDate, Long accountId, List<Long> domainIds,
+ long limitPerQuery) {
+ SearchCriteria<EventVO> sc = createEventSearchCriteria(ids, type,
startDate, endDate, null, accountId, domainIds);
+ Filter filter = null;
+ if (limitPerQuery > 0) {
+ filter = new Filter(limitPerQuery);
+ }
+
+ long archived;
+ long totalArchived = 0L;
+
+ do {
+ List<EventVO> events = search(sc, filter);
+ if (events.isEmpty()) {
+ break;
}
- txn.close();
+
+ archived = archiveEventsInternal(events);
+ totalArchived += archived;
+ } while (limitPerQuery > 0 && archived >= limitPerQuery);
Review Comment:
The loop termination condition uses the number of rows updated (`archived`)
to decide whether to fetch the next batch. JDBC drivers/DBs can report fewer
affected rows than selected rows (e.g., if some rows are updated concurrently
or if an update sets the column to its current value), which can prematurely
stop batching while more unarchived events still match the criteria. Consider
basing the loop continuation on the batch size returned by `search` (e.g.,
`events.size() == limitPerQuery`) instead of the update count, or otherwise
ensure the termination condition can’t end early.
--
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]
