Truncate API added by Sreemanth · Pull Request #5573 · apache/pinot

Sreemanth · 2020-06-16T12:59:13Z

Description

#5559: Truncate API Added.

Offline : Disabled Table and deleted all segments
RealTime: Disabled Table and deleted all segments.

@Jackie-Jiang
Implemented as per your comment. (PinotSegmentRestletResource.deleteAllSegments())

codecov-commenter · 2020-06-16T15:36:44Z

Codecov Report

Merging #5573 into master will decrease coverage by 0.17%.
The diff coverage is 68.03%.

@@            Coverage Diff             @@
##           master    #5573      +/-   ##
==========================================
- Coverage   66.44%   66.27%   -0.18%     
==========================================
  Files        1075     1123      +48     
  Lines       54773    57490    +2717     
  Branches     8168     8609     +441     
==========================================
+ Hits        36396    38101    +1705     
- Misses      15700    16560     +860     
- Partials     2677     2829     +152

Flag	Coverage Δ
#integrationtests	`45.18% <45.96%> (?)`
#unittests	`56.88% <60.23%> (?)`

Impacted Files	Coverage Δ
...quota/HelixExternalViewBasedQueryQuotaManager.java	`67.87% <0.00%> (ø)`
...org/apache/pinot/common/function/FunctionInfo.java	`73.33% <ø> (ø)`
...not/common/lineage/SegmentLineageAccessHelper.java	`0.00% <0.00%> (ø)`
...ache/pinot/common/metadata/ZKMetadataProvider.java	`66.85% <0.00%> (ø)`
...java/org/apache/pinot/common/segment/ReadMode.java	`66.66% <ø> (ø)`
...org/apache/pinot/common/utils/CommonConstants.java	`39.02% <0.00%> (+0.92%)`	⬆️
...oller/api/resources/PinotTableRestletResource.java	`51.48% <0.00%> (-5.06%)`	⬇️
...troller/helix/core/retention/RetentionManager.java	`80.28% <0.00%> (+1.11%)`	⬆️
...he/pinot/controller/util/AutoAddInvertedIndex.java	`0.00% <0.00%> (ø)`
.../org/apache/pinot/core/common/BaseBlockValSet.java	`3.03% <0.00%> (-1.32%)`	⬇️
... and 478 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3a8368...e74a129. Read the comment docs.

siddharthteotia · 2020-06-22T17:42:56Z

    return new SuccessResponse("Table config updated for " + tableName);
  }
+
+  @POST


Can we please add documentation highlighting the differences between this and table drop?

Generally, the standard implemented by other databases has the following distinction:

DROP will delete table data, indexes, metadata etc. So any statement (to fetch data or query metadata) will fail stating "table does not exist"

TRUNCATE will delete table data, indexes, but the metadata will be kept around. So you can still see table's schema, table config etc and the queries will return empty results AFAIK.

Are we following the same distinction? If so, we should update the docs to state when should the user choose DROP vs TRUNCATE.

Also, we should add tests to ensure that both behave expectedly. If TRUNCATE is not expected to delete config and schema, then we should add a test for that.

+1. drop and truncate behavior should follow the standards.

npawar · 2020-06-24T20:44:11Z

+  @ApiOperation(value = "Truncate table")
+  public SuccessResponse truncateTable(
+      @ApiParam(value = "Name of the table to update", required = true) @PathParam("tableName") String tableName,
+      @ApiParam(value = "realtime|offline") @QueryParam("type") String tableTypeStr) throws Exception {


We still have to define what truncate means for realtime. Typically, when you create a realtime table, it immediately starts consumption. So on truncate, after dropping all segments, we should again restart consumption based on configured offset.
That will not happen with these changes. Consumption will be restarted only on the next run of the RealtimeSegmentValidationManager. Which I don't think is an acceptable flow.

3 options

this is ok as part of the version

we can invoke the validation manager at the end of the call

we can add an separate call to pause/resume Kafka consumption

Can we take the discussion to the Issue? I believe we need to agree on a spec first before we start publishing PRs.

npawar · 2020-06-24T20:53:57Z

I think this task is still pending discussion about race conditions between this API and segment completion/uploads. The issue has more details

mcvsubbu · 2020-06-24T20:58:54Z

Can we take the conversation to the Issue instead of this PR?

mcvsubbu · 2020-06-24T23:59:42Z

@kishoreg we cannot invoke validation manager unless we get to the lead controller for that particular table. With controller separation using helix controller management, all controllers take part in valdation. etc.

mcvsubbu · 2020-06-22T20:26:24Z

+  @Path("/tables/{tableName}/truncate")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "Truncate table")
+  public SuccessResponse truncateTable(


See #5559 (comment)

Let us first agree that adding a new API (that we need to keep backward compatible, and so on) is the best answer to the problem.

mcvsubbu · 2020-06-25T00:01:12Z

+  @ApiOperation(value = "Truncate table")
+  public SuccessResponse truncateTable(
+      @ApiParam(value = "Name of the table to update", required = true) @PathParam("tableName") String tableName,
+      @ApiParam(value = "realtime|offline") @QueryParam("type") String tableTypeStr) throws Exception {


Can we take the discussion to the Issue? I believe we need to agree on a spec first before we start publishing PRs.

npawar · 2020-07-26T23:17:09Z

+    toggleTableState(tableName, tableType, StateType.DISABLE);
+
+    // Get all segment names for table and delete all segments
+    String tableNameWithType = getExistingTableNamesWithType(tableName, tableType).get(0);


How about moving line String tableNameWithType = getExistingTableNamesWithType(tableName, tableType).get(0); before the toggle, so that we are validating if table exists before calling disable.

Then you can also directly call _pinotHelixResourceManager.toggleTableState(tableNameWithType, status);

npawar · 2020-07-26T23:19:17Z

+    // Get all segment names for table and delete all segments
+    String tableNameWithType = getExistingTableNamesWithType(tableName, tableType).get(0);
+    List<String> segmentNames = _pinotHelixResourceManager.getSegmentsFor(tableNameWithType);
+    PinotResourceManagerResponse response = _pinotHelixResourceManager.deleteSegments(tableNameWithType, segmentNames);


before deleting segments, you need to check that all segments went into OFFLINE state in the external view. Pasting the steps Kishore had suggested in the issue:

disable the table wait for all segments to go to offline state delete all the segments enable the table call the setup table method

npawar · 2020-07-26T23:22:34Z

+    toggleTableState(tableName, tableType, StateType.ENABLE);
+
+    // Setup table for real-time : (ensureRealtimeClusterIsSetUp)
+    if (tableType != TableType.OFFLINE && _pinotHelixResourceManager.hasRealtimeTable(tableName)) {


why not just if(tableType == REALTIME) ?
and you already have tableNameWithType variable above, no need to construct the tableName again

npawar · 2020-07-26T23:24:45Z

    }
  }
+
+  private List<String> getExistingTableNamesWithType(String tableName, @Nullable TableType tableType) {


tableType doesn't need annotation Nullable - you're checking for not-null before this call

npawar · 2020-07-26T23:25:34Z

@@ -1240,7 +1240,7 @@ private void verifyIndexingConfig(String tableNameWithType, IndexingConfig index
    }
  }



Since we made this a public method, can you add some javadocs explaining what this method does?

npawar · 2020-07-26T23:26:55Z

  }
+
+  /**
+   * Truncate table, delete all contents of table without removing table configuration and schema. 


Let's add here that for Truncate for REALTIME includes creating new CONSUMING segments in segment metadata and ideal state

npawar · 2020-07-26T23:28:55Z

+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "Truncate table")
+  public SuccessResponse truncateTable(
+      @ApiParam(value = "Name of the table to update", required = true) @PathParam("tableName") String tableName,


Name of table to "truncate" (not update)

Let's make "type" also a required param

npawar · 2020-07-26T23:33:09Z

  }
+
+  @Test
+  public void testTruncateTable() throws IOException {


we need some more testing

create a OFFLINE table and schema, add some segments, then call truncate. Check that segments/idealstate/external view are deleted, but the tableconfig and schema are intact

create a REALTIME table and schema, let it create some CONSUMING segments, then call truncate. Check that table config and schema re intact. Also check that idealstate/external view/segments now have the newly created segments.

Truncate API added

e74a129

kishoreg requested a review from npawar June 16, 2020 16:43

siddharthteotia reviewed Jun 22, 2020

View reviewed changes

sreemanthpulagam added 2 commits June 24, 2020 14:19

Added documentation and unit test case for Truncate (apache#5559)

575b1e4

Merge remote-tracking branch 'origin/master' into truncate_command

7f825e6

npawar reviewed Jun 24, 2020

View reviewed changes

mcvsubbu reviewed Jun 25, 2020

View reviewed changes

mcvsubbu self-requested a review June 25, 2020 00:01

sreemanthpulagam added 7 commits June 26, 2020 03:02

Truncate: Fixed failing testcase (apache#5559)

4d00a83

Merge branch 'master' into truncate_command

6a37ecf

Enabling table after deleting segments (apache#5559)

7cb5bd1

Moved enable or disable to common method (apache#5559)

b1e0506

Fixed enable table state (apache#5559)

5138191

Truncate table: Ensure realtime table cluster setup (apache#5559)

2ef2407

Merge branch 'master' into truncate_command

6422f57

npawar reviewed Jul 26, 2020

View reviewed changes

		@@ -1240,7 +1240,7 @@ private void verifyIndexingConfig(String tableNameWithType, IndexingConfig index
		}
		}

Conversation

Sreemanth commented Jun 16, 2020

Description

Uh oh!

codecov-commenter commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

npawar commented Jun 24, 2020

Uh oh!

mcvsubbu commented Jun 24, 2020

Uh oh!

mcvsubbu commented Jun 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented Jun 16, 2020 •

edited

Loading