Damans227 commented on code in PR #13566:
URL: https://github.com/apache/cloudstack/pull/13566#discussion_r3624821931
##########
api/src/main/java/org/apache/cloudstack/query/QueryService.java:
##########
@@ -140,6 +140,12 @@ public interface QueryService {
ConfigKey<Boolean> ReturnVmStatsOnVmList = new ConfigKey<>("Advanced",
Boolean.class, "list.vm.default.details.stats", "true",
"Determines whether VM stats should be returned when details are
not explicitly specified in listVirtualMachines API request. When false,
details default to [group, nics, secgrp, tmpl, servoff, diskoff, backoff, iso,
volume, min, affgrp]. When true, all details are returned including 'stats'.",
true, ConfigKey.Scope.Global);
+ ConfigKey<Boolean> BypassTemplateView = new ConfigKey<>("Advanced",
Boolean.class, "template.list.bypass.view",
+ "false",
+ "If true, uses an optimized query path for listing templates and
ISOs, which can improve performance on " +
Review Comment:
Nit: the concatenated description string has inconsistent leading/trailing
space placement across the lines, worth double-checking the rendered text
doesn't end up with a missing or doubled space.
##########
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:
##########
@@ -44,6 +44,14 @@
import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao;
import com.cloud.cluster.ManagementServerHostPeerJoinVO;
+import com.cloud.template.VirtualMachineTemplate;
Review Comment:
Nit: these new imports break alphabetical order here, and the same classes
(`AccountVO`, `SSHKeyPairVO`, `SSHKeyPairDao`, `InstanceGroupVMMapVO`, `NicVO`,
`InstanceGroupVMMapDao`, `NicDao`) get removed from their previously-correct
alphabetized spots further down in this same diff.
##########
server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java:
##########
@@ -703,6 +709,288 @@ public Pair<List<TemplateJoinVO>, Integer>
searchIncludingRemovedAndCount(final
return new Pair<List<TemplateJoinVO>, Integer>(objects, count);
}
+ //
============================================================================
+ // The standard Phase 1 path runs `SELECT DISTINCT temp_zone_pair FROM
+ // template_view WHERE ...` against a 13-table view.
+ //
+ // This bypass path issues hand-tuned SQL against only the 6 tables needed
+ // to compute the (template_id, data_center_id) pair: vm_template, account,
+ // template_store_ref, image_store, template_zone_ref, data_center. The OR
+ // join is replaced with COALESCE.
+ //
+ // Hard filters (tags, sharedAccountIds, domainPath, featured/community
+ // domain hierarchy) are not implemented here —
TemplateListFilter#canBypass()
+ // returns false in those cases and the dispatcher falls back to the
+ // SearchBuilder path.
+
+ private static final Field TEMPLATE_JOIN_ID_FIELD;
+ private static final Field TEMPLATE_JOIN_PAIR_FIELD;
+
+ static {
+ try {
+ TEMPLATE_JOIN_ID_FIELD =
findFieldUpHierarchy(TemplateJoinVO.class, "id");
+ TEMPLATE_JOIN_PAIR_FIELD =
findFieldUpHierarchy(TemplateJoinVO.class, "tempZonePair");
+ TEMPLATE_JOIN_ID_FIELD.setAccessible(true);
+ TEMPLATE_JOIN_PAIR_FIELD.setAccessible(true);
+ } catch (NoSuchFieldException e) {
+ throw new ExceptionInInitializerError(e);
+ }
+ }
+
+ private static Field findFieldUpHierarchy(Class<?> clazz, String name)
throws NoSuchFieldException {
+ Class<?> c = clazz;
+ while (c != null) {
+ try {
+ return c.getDeclaredField(name);
+ } catch (NoSuchFieldException ignored) {
+ c = c.getSuperclass();
+ }
+ }
+ throw new NoSuchFieldException(name + " on " + clazz);
+ }
+
+ @Override
+ public Pair<List<TemplateJoinVO>, Integer>
findDistinctTempZonePairs(TemplateListFilter filter) {
+ if (!filter.canBypass()) {
+ throw new IllegalArgumentException(
+ "findDistinctTempZonePairs called with unsupported filter
(tags / sharedAccountIds / domainPath / domainIds populated). Caller should
fall back to searchAndDistinctCount.");
+ }
+
+ List<Object> params = new ArrayList<>();
+ StringBuilder where = new StringBuilder(" WHERE 1=1");
+ appendCommonWhere(where, params, filter);
+
+ String fromClause = buildFromClause(filter);
+
+ String selectExpr = filter.showUnique
+ ? "vt.id"
+ : "CONCAT(vt.id, '_', IFNULL(dc.id, 0))";
+
+ String dataSql = "SELECT DISTINCT " + selectExpr + " AS distinct_key,
vt.sort_key"
+ + " FROM " + fromClause
+ + where
+ + " ORDER BY vt.sort_key " + (filter.sortAscending ? "ASC" :
"DESC")
+ + ", distinct_key " + (filter.sortAscending ? "ASC" : "DESC")
+ + buildLimitClause(filter);
+
+ String countSql = "SELECT COUNT(DISTINCT " + selectExpr + ")"
+ + " FROM " + fromClause
+ + where;
+
+ List<TemplateJoinVO> rows = executeDistinctQuery(dataSql, params,
filter.showUnique);
+ int count = executeCountQuery(countSql, params);
+ return new Pair<>(rows, count);
+ }
+
+ /**
+ * Build the FROM clause. The 6-table base is always present; conditional
+ * joins are added when the filter actually needs them. Today only
+ * `domain` is conditional (used for {@code domainPathLike} predicates);
+ * `launch_permission` and `resource_tags` are added in later iterations.
+ */
+ private String buildFromClause(TemplateListFilter filter) {
+ StringBuilder from = new StringBuilder()
+ .append("cloud.vm_template vt")
Review Comment:
Nit: hardcoding the `cloud.` schema prefix here is unusual for this
codebase's DAOs, worth checking this holds for all deployments.
##########
server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java:
##########
@@ -703,6 +709,288 @@ public Pair<List<TemplateJoinVO>, Integer>
searchIncludingRemovedAndCount(final
return new Pair<List<TemplateJoinVO>, Integer>(objects, count);
}
+ //
============================================================================
+ // The standard Phase 1 path runs `SELECT DISTINCT temp_zone_pair FROM
+ // template_view WHERE ...` against a 13-table view.
+ //
+ // This bypass path issues hand-tuned SQL against only the 6 tables needed
+ // to compute the (template_id, data_center_id) pair: vm_template, account,
+ // template_store_ref, image_store, template_zone_ref, data_center. The OR
+ // join is replaced with COALESCE.
+ //
+ // Hard filters (tags, sharedAccountIds, domainPath, featured/community
+ // domain hierarchy) are not implemented here —
TemplateListFilter#canBypass()
+ // returns false in those cases and the dispatcher falls back to the
+ // SearchBuilder path.
+
+ private static final Field TEMPLATE_JOIN_ID_FIELD;
+ private static final Field TEMPLATE_JOIN_PAIR_FIELD;
+
+ static {
+ try {
+ TEMPLATE_JOIN_ID_FIELD =
findFieldUpHierarchy(TemplateJoinVO.class, "id");
+ TEMPLATE_JOIN_PAIR_FIELD =
findFieldUpHierarchy(TemplateJoinVO.class, "tempZonePair");
+ TEMPLATE_JOIN_ID_FIELD.setAccessible(true);
Review Comment:
Nit: reflection to populate `TemplateJoinVO`'s private fields feels like a
fairly invasive way to build this VO, a package-visible setter would be less
fragile against future field renames.
##########
server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java:
##########
@@ -703,6 +709,288 @@ public Pair<List<TemplateJoinVO>, Integer>
searchIncludingRemovedAndCount(final
return new Pair<List<TemplateJoinVO>, Integer>(objects, count);
}
+ //
============================================================================
Review Comment:
Nit: this banner-style comment block doesn't match the rest of the
codebase's usual `/** javadoc */` or short `//` comment style.
--
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]
