Pagination #17638

Lindsay-00 · 2025-03-19T22:24:19Z

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2025-03-19T22:46:33Z

❌ Gradle check result for ad612da: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2025-03-22T08:51:10Z

❌ Gradle check result for baa30b0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

kaushalmahi12 · 2025-03-21T19:21:40Z

server/src/main/java/org/opensearch/action/pagination/WlmPaginationStrategy.java

+    private final int pageSize;
+    private final String nextToken;
+    private final String sortBy;
+    private final String sortOrder;


I think we should make sortBy and sortOrder an Enum since this is going to be a limited set of values and move the switch case logic inside the enum.

enum SortBy { case QUERY_GROUP, NODE_ID; Comparator<WlmStats> comparator; .... public Comparator<WlmStats> getComparator() { return comparator; } } enum SortOrder { case ASC, DESC; Function<Comparator<WlmStats>, Comparator<WlmStats>> transformer; .... public Comparator<WlmStats> transform(Comparator<WlmStats> comp) { return transformer.apply(comp); } }

kaushalmahi12 · 2025-03-24T22:09:56Z

server/src/main/java/org/opensearch/action/pagination/WlmPaginationStrategy.java

+            return Collections.emptyList();
+        }
+
+        List<WlmStats> allQueryGroups = new ArrayList<>();


This var name is misleading as this holds the stats not the queryGroups.

kaushalmahi12 · 2025-03-24T22:34:07Z

server/src/main/java/org/opensearch/action/pagination/WlmPaginationStrategy.java

+            }
+        }
+
+        allQueryGroups = sortStats(allQueryGroups, sortBy, sortOrder);


This is not needed given that we already have the sorted list of WLM stats.

kaushalmahi12 · 2025-03-24T22:35:04Z

server/src/main/java/org/opensearch/action/pagination/WlmPaginationStrategy.java

+
+        int startIndex = 0;
+        if (requestedToken != null) {
+            OptionalInt foundIndex = findIndex(allQueryGroups, requestedToken.lastQueryGroupId);


Filtering based on repetitive filed can result into circular loops of results. for example the same set of query group id will be present on all WlmStats entries and if your pageSize and querygroup count are equal then it will always return first two pages worth of results.

kaushalmahi12 · 2025-03-24T22:40:10Z

server/src/main/java/org/opensearch/common/Table.java

@@ -245,6 +245,11 @@ public PageToken getPageToken() {
        return pageToken;
    }

+    public void setPageToken(PageToken pageToken) {


I think this is not needed, We can use the constructor to inject the token.

kaushalmahi12 · 2025-03-24T23:19:33Z

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestWlmStatsAction.java

+                QueryGroupStats.ResourceStats cpuStats = statsHolder.getResourceStats().get(ResourceType.CPU);
+                QueryGroupStats.ResourceStats memoryStats = statsHolder.getResourceStats().get(ResourceType.MEMORY);
+
+                table.addCell(cpuStats != null ? cpuStats.getCurrentUsage() : 0);


I think 0 would be a misnomer here instead put a placeholder like _ or NA.

kaushalmahi12 · 2025-03-24T23:20:02Z

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestWlmStatsAction.java

+        if (paginationStrategy.getNextToken() == null) {
+            table.startRow();
+            table.addCell("No more pages available");
+            for (int i = 1; i < 13; i++) {


We should make the literals as named constants

kaushalmahi12 · 2025-03-24T23:21:59Z

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestWlmStatsAction.java

+            table.startRow();
+            table.addCell("No more pages available");
+            for (int i = 1; i < 13; i++) {
+                table.addCell("-");
+            }
+            table.endRow();


Should this be conditional ? Additionally lets refactor a method for this footer .

kaushalmahi12 · 2025-03-24T23:22:37Z

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestWlmStatsAction.java

+                String queryGroupId = entry.getKey();
+                QueryGroupStats.QueryGroupStatsHolder statsHolder = entry.getValue();
+
+                table.startRow();
+                table.addCell(wlmStats.getNode().getId());
+                table.addCell("|");
+                table.addCell(queryGroupId);
+                table.addCell("|");
+                table.addCell(statsHolder.getCompletions());
+                table.addCell("|");
+                table.addCell(statsHolder.getRejections());
+                table.addCell("|");
+                table.addCell(statsHolder.getCancellations());
+                table.addCell("|");
+
+                QueryGroupStats.ResourceStats cpuStats = statsHolder.getResourceStats().get(ResourceType.CPU);
+                QueryGroupStats.ResourceStats memoryStats = statsHolder.getResourceStats().get(ResourceType.MEMORY);
+
+                table.addCell(cpuStats != null ? cpuStats.getCurrentUsage() : 0);
+                table.addCell("|");
+                table.addCell(memoryStats != null ? memoryStats.getCurrentUsage() : 0);
+                table.endRow();


We can refactor these lines into a method named addRow(table);

kaushalmahi12 · 2025-03-24T23:23:32Z

server/src/test/java/org/opensearch/action/pagination/DiscoveryNodeMock.java

I think this file is not needed as we can directly instantiate the DiscoverNode instance with mock transport.

github-actions · 2025-03-26T06:21:25Z

❌ Gradle check result for 0dd053d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

ruai0511 · 2025-03-27T05:46:41Z

server/src/main/java/org/opensearch/common/Table.java

@@ -245,6 +245,7 @@ public PageToken getPageToken() {
        return pageToken;
    }

+


Let's remove this blank line

ruai0511 · 2025-03-27T05:50:13Z

server/src/main/java/org/opensearch/action/pagination/WlmPaginationStrategy.java

+            Objects.requireNonNull(token, "Token cannot be null");
+            String decrypted = PaginationStrategy.decryptStringToken(token);
+            final String[] parts = decrypted.split(SPLIT_REGEX);
+            if (parts.length != 4 || parts[0].isEmpty() || parts[1].isEmpty() || parts[3].isEmpty()) {


I saw you defined

private static final int NODE_ID_POS = 0; private static final int QUERY_GROUP_ID_POS = 1; private static final int QUERY_GROUP_COUNT_POS = 2; private static final int HASH_POS = 3;

We should replace the numbers here with these static fields.

ruai0511 · 2025-03-27T05:52:05Z

server/src/main/java/org/opensearch/action/pagination/WlmPaginationStrategy.java

+        }
+    }
+
+    private List<WlmStats> applyPagination(List<WlmStats> rawStats, WlmStrategyToken requestedToken, String currentHash) {


This function is too long. Let's break it into several shorter functions. Also add a javadoc to explain how the pagination works for better readability

ruai0511 · 2025-03-27T05:57:26Z

server/src/main/java/org/opensearch/action/pagination/WlmPaginationStrategy.java

+        return paginatedStats;
+    }
+
+    public PageToken getNextToken() {


Duplication. The function getResponseToken() returns responseToken as well.

ruai0511 · 2025-03-27T06:10:10Z

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestWlmStatsAction.java

+                            builder.field("details", e.getMessage());
+                            builder.endObject();
+
+                            channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, builder));


Should add logging information (using logger.error) before throwing an exception to provide better debugging information for clients.

ruai0511 · 2025-03-27T06:15:17Z

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestWlmStatsAction.java

+                        try {
+                            WlmPaginationStrategy paginationStrategy = new WlmPaginationStrategy(pageSize, nextToken, sortBy, sortOrder, response);
+                            List<WlmStats> paginatedStats = paginationStrategy.getPaginatedStats();
+                            PageToken nextPageToken = paginationStrategy.getNextToken();
+
+                            // Use constructor to create Table with token
+                            Table paginatedTable = createTableWithHeaders(nextPageToken);
+                            buildTable(paginatedTable, paginatedStats, paginationStrategy);
+
+                            request.params().put("v", "true");
+                            return RestTable.buildResponse(paginatedTable, channel);
+                        } catch (OpenSearchParseException e) {
+                            String userMessage = "Pagination state has changed (e.g., new query groups added or removed). "
+                                + "Please restart pagination from the beginning by omitting the 'next_token' parameter.";
+
+                            XContentBuilder builder = XContentFactory.jsonBuilder();
+                            builder.startObject();
+                            builder.field("error", userMessage);
+                            builder.field("details", e.getMessage());
+                            builder.endObject();
+
+                            channel.sendResponse(new BytesRestResponse(RestStatus.BAD_REQUEST, builder));
+                            return null;


Extract these lines in a new method to improve the readability of functionprepareRequest

ruai0511 · 2025-03-27T06:59:22Z

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestWlmStatsAction.java

+        Boolean breach = request.hasParam("breach") ? Boolean.parseBoolean(request.param("breach")) : null;
+        WlmStatsRequest wlmStatsRequest = new WlmStatsRequest(nodeIds, queryGroupIds, breach);
+
+        int pageSize = request.paramAsInt("size", 10);


Do we have validation logic for pageSize? eg. greater than 0 and less than a certain value to ensure performance.

ruai0511 · 2025-03-27T07:07:21Z

server/src/main/java/org/opensearch/action/pagination/SortBy.java

+        switch (input.toLowerCase()) {
+            case "query_group":
+                return QUERY_GROUP;
+            case "node_id":
+                return NODE_ID;
+            default:
+                throw new IllegalArgumentException("Invalid sort field: " + input);
+        }


Can we use this to avoid using switch? Same for SortOrder

try { return SortBy.valueOf(input.toUpperCase()); } catch (IllegalArgumentException e) { throw new IllegalArgumentException("Invalid sort field: " + input + ". Allowed values: query_group, node_id"); }

ruai0511 · 2025-03-27T07:09:50Z

Can we provide a better name for this PR and add some description? A single word title is not descriptive

Lingxi Chen added 6 commits March 17, 2025 03:12

tabular format

f7b9243

pagination

cc05920

tests unfinished

bfc90b7

unit tests

44f5e10

fixed apply pagination

0f4cf5b

deleting

58ceeb1

Lingxi Chen added 2 commits March 19, 2025 15:32

deleting files

ed4336b

more

ad612da

yaml tests

baa30b0

kaushalmahi12 reviewed Mar 24, 2025

View reviewed changes

changed based on comments except for testing

0dd053d

ruai0511 reviewed Mar 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pagination #17638

Pagination #17638

Lindsay-00 commented Mar 19, 2025

github-actions bot commented Mar 19, 2025

github-actions bot commented Mar 22, 2025

kaushalmahi12 Mar 21, 2025

kaushalmahi12 Mar 24, 2025

kaushalmahi12 Mar 24, 2025

kaushalmahi12 Mar 24, 2025

kaushalmahi12 Mar 24, 2025

kaushalmahi12 Mar 24, 2025

kaushalmahi12 Mar 24, 2025

kaushalmahi12 Mar 24, 2025

kaushalmahi12 Mar 24, 2025

kaushalmahi12 Mar 24, 2025

github-actions bot commented Mar 26, 2025

ruai0511 Mar 27, 2025

ruai0511 Mar 27, 2025

ruai0511 Mar 27, 2025

ruai0511 Mar 27, 2025

ruai0511 Mar 27, 2025

ruai0511 Mar 27, 2025

ruai0511 Mar 27, 2025

ruai0511 Mar 27, 2025

ruai0511 commented Mar 27, 2025

		@@ -245,6 +245,7 @@ public PageToken getPageToken() {
		return pageToken;
		}

Pagination #17638

Are you sure you want to change the base?

Pagination #17638

Conversation

Lindsay-00 commented Mar 19, 2025

Description

Related Issues

Check List

github-actions bot commented Mar 19, 2025

github-actions bot commented Mar 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruai0511 commented Mar 27, 2025