Add table indexes API #11576

saurabhd336 · 2023-09-12T11:22:32Z

This API is primarily for the controller UI -> tables ->

-> reload status -> STATUS ui element. Right now it uses the /segments/{tableName}/metadata?type={type}&columns=* API to display the column level index details. This has a few problems

This is an expensive API call especially for large tables. This leads to bad ux when using the reload status UI feature
The UI logic to build tableLevel column -> index mapping using this segmentName level metadata is simply wrong (uss the first segment's metadata to build the view).
Since Index-spi changes are merged, this API does not return metadata of dynamically added index types, but only some hardcoded ones.

This new API's reponse looks like this

The totalSegments is the total count of ONLINE segments (replication included). Since mutable version of all indexes don't exist, CONSUMING segments will not be used when building the response.

{
    "totalSegments": 31,
    "columnToIndexesCount":
    {
        "FlightNum":
        {
            "dictionary": 31,
            "bloom": 0,
            "null": 0,
            "forward": 31,
            "fst": 0,
            "json": 0,
            "range": 0,
            "h3": 0,
            "text": 0,
            "inverted": 0,
            "some-dynamically-injected-index-type": 31,
        },
        "Origin":
        {
            "dictionary": 31,
            "bloom": 0,
            "null": 0,
            "forward": 31,
            "fst": 0,
            "json": 0,
            "range": 0,
            "h3": 0,
            "text": 0,
            "inverted": 0,
            "some-dynamically-injected-index-type": 0,
        }
        ...
}

I'll followup with @jayeshchoudhary to move the UI API call to this new one, and how best to display the UI element once this PR is merged.

saurabhd336 · 2023-09-12T11:24:13Z

API calls involved when viewing this modal. For a large enough table, this UI element takes ~40s to load

codecov-commenter · 2023-09-12T12:06:32Z

Codecov Report

Merging #11576 (75f1b45) into master (b1b070f) will increase coverage by 0.02%.
The diff coverage is 30.43%.

@@             Coverage Diff              @@
##             master   #11576      +/-   ##
============================================
+ Coverage     14.51%   14.54%   +0.02%     
  Complexity      201      201              
============================================
  Files          2323     2324       +1     
  Lines        124465   124534      +69     
  Branches      18989    18994       +5     
============================================
+ Hits          18071    18109      +38     
- Misses       104877   104906      +29     
- Partials       1517     1519       +2

Flag	Coverage Δ
integration	`?`
integration2	`?`
java-11	`14.52% <30.43%> (+<0.01%)`	⬆️
java-17	`14.51% <30.43%> (+<0.01%)`	⬆️
java-20	`14.53% <30.43%> (+0.02%)`	⬆️
temurin	`14.54% <30.43%> (+0.02%)`	⬆️
unittests	`14.54% <30.43%> (+0.02%)`	⬆️
unittests2	`14.54% <30.43%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...on/response/server/TableIndexMetadataResponse.java	`0.00% <0.00%> (ø)`
...oller/api/resources/PinotTableRestletResource.java	`43.42% <0.00%> (-3.96%)`	⬇️
...che/pinot/server/api/resources/TablesResource.java	`60.98% <91.30%> (+2.47%)`	⬆️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ege-st · 2023-09-12T15:56:37Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "Get the aggregate index details of all segments for a table", notes = "Get the aggregate "
+      + "index details of all segments for a table")
+  public String getTableIndexes(


Since this is a new API, shouldn't we have tests to validate its behavior?

Ack. added tests

ege-st · 2023-09-12T15:57:33Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+      response = ErrorInfo.class), @ApiResponse(code = 404, message = "Table or segment not found", response =
+      ErrorInfo.class)
+  })
+  public String getTableIndexes(


Likewise, shouldn't we have some unit tests and integration tests to validate this new API?

Ack. added tests

ege-st

Had one question about tests.

ege-st · 2023-09-13T14:34:48Z

pinot-server/src/test/java/org/apache/pinot/server/api/TablesResourceTest.java

+  public void getTableIndexes()
+      throws Exception {
+    String tableIndexesPath =
+        "/tables/" + TableNameBuilder.forType(TableType.OFFLINE).tableNameWithType(TABLE_NAME) + "/indexes";


Do we also need to test this for RT tables?

Jackie-Jiang

LGTM

Jackie-Jiang · 2023-09-17T19:46:31Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+      long totalSegmentCount = 0;
+      Map<String, Map<String, Long>> columnToIndexesCount = new HashMap<>();
+      for (SegmentDataManager segmentDataManager : allSegments) {
+        if (segmentDataManager instanceof RealtimeSegmentDataManager) {


In the documentation, let's mention that consuming segment is not counted to avoid confusion

Jackie-Jiang · 2023-09-17T19:47:28Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+    TableDataManager tableDataManager = ServerResourceUtils.checkGetTableDataManager(_serverInstance, tableName);
+    List<SegmentDataManager> allSegments = tableDataManager.acquireAllSegments();
+    try {
+      long totalSegmentCount = 0;


(minor) long seems not necessary, same for the index count

Jackie-Jiang · 2023-09-17T19:51:41Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

+        columnToIndexCountMap.putIfAbsent(col, new HashMap<>());
+
+        indexToCount.forEach((indexName, count) -> {
+          columnToIndexCountMap.get(col)
+              .put(indexName, columnToIndexCountMap.get(col).getOrDefault(indexName, 0L) + count);
+        });


Reduce map lookup

Suggested change

columnToIndexCountMap.putIfAbsent(col, new HashMap<>());

indexToCount.forEach((indexName, count) -> {

columnToIndexCountMap.get(col)

.put(indexName, columnToIndexCountMap.get(col).getOrDefault(indexName, 0L) + count);

});

Map<String, Integer> indexCountMap = columnToIndexCountMap.computeIfAbsent(col, new HashMap<>());

indexToCount.forEach((indexName, count) -> {

indexCountMap.compute(col, (k, v) -> v != null ? v + count : count);

});

ege-st reviewed Sep 12, 2023

View reviewed changes

saurabhd336 requested a review from ege-st September 13, 2023 05:44

ege-st approved these changes Sep 13, 2023

View reviewed changes

Jackie-Jiang added feature rest-api documentation labels Sep 17, 2023

Jackie-Jiang approved these changes Sep 17, 2023

View reviewed changes

saurabhd336 force-pushed the tableIndexesApi branch from def1e04 to 0b9a468 Compare September 18, 2023 04:10

Saurabh Dubey added 6 commits September 18, 2023 10:17

Add table indexes API

1970eff

Add tests

b58b31c

Lint fix

417ca5c

Fix test

6da6c1d

Address comments

66b3b83

Fix test

a4e1370

saurabhd336 force-pushed the tableIndexesApi branch from 0b9a468 to a4e1370 Compare September 18, 2023 04:49

Fix npe

75f1b45

saurabhd336 merged commit 49196ec into apache:master Sep 18, 2023
21 checks passed

jayeshchoudhary mentioned this pull request Oct 31, 2023

UI: show partial index in reload status #11913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add table indexes API #11576

Add table indexes API #11576

saurabhd336 commented Sep 12, 2023 •

edited

Loading

saurabhd336 commented Sep 12, 2023

codecov-commenter commented Sep 12, 2023 •

edited

Loading

ege-st Sep 12, 2023

saurabhd336 Sep 13, 2023

ege-st Sep 12, 2023

saurabhd336 Sep 13, 2023

ege-st left a comment

ege-st Sep 13, 2023

Jackie-Jiang left a comment

Jackie-Jiang Sep 17, 2023

saurabhd336 Sep 18, 2023

Jackie-Jiang Sep 17, 2023

saurabhd336 Sep 18, 2023

Jackie-Jiang Sep 17, 2023

saurabhd336 Sep 18, 2023

Add table indexes API #11576

Add table indexes API #11576

Conversation

saurabhd336 commented Sep 12, 2023 • edited Loading

saurabhd336 commented Sep 12, 2023

codecov-commenter commented Sep 12, 2023 • edited Loading

Codecov Report

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

ege-st left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang left a 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

saurabhd336 commented Sep 12, 2023 •

edited

Loading

codecov-commenter commented Sep 12, 2023 •

edited

Loading