vvivekiyer commented on a change in pull request #8110:
URL: https://github.com/apache/pinot/pull/8110#discussion_r798237731
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/ZKOperator.java
##########
@@ -222,8 +222,16 @@ private void checkCRC(HttpHeaders headers, String
offlineTableName, String segme
private void processNewSegment(SegmentMetadata segmentMetadata, URI
finalSegmentLocationURI,
File currentSegmentLocation, String zkDownloadURI, HttpHeaders headers,
String crypter, String tableNameWithType,
- String segmentName, boolean moveSegmentToFinalLocation)
+ String segmentName, boolean moveSegmentToFinalLocation, boolean
enableParallelPushProtection)
throws Exception {
+ SegmentZKMetadata newSegmentZKMetadata = _pinotHelixResourceManager
+ .constructZkMetadataForNewSegment(tableNameWithType, segmentMetadata,
zkDownloadURI, crypter,
+ enableParallelPushProtection);
+ if (!_pinotHelixResourceManager.updateZkMetadata(tableNameWithType,
newSegmentZKMetadata)) {
Review comment:
Thanks for this comment. As Jackie pointed out, expected version -1
could end up overwriting anyway if there are concurrent calls.
To avoid this we can use ZkCacheBaseDataAccessor.set(). This will only allow
one call to succeed and fail the second.
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/api/upload/ZKOperatorTest.java
##########
@@ -64,11 +64,34 @@ public void testCompleteSegmentOperations()
when(segmentMetadata.getCrc()).thenReturn("12345");
when(segmentMetadata.getIndexCreationTime()).thenReturn(123L);
HttpHeaders httpHeaders = mock(HttpHeaders.class);
+
+ // Test if Zk segment metadata is removed if exception is thrown when
moving segment to final location.
+ try {
+ // Create mock finalSegmentLocationURI and currentSegmentLocation.
+ URI finalSegmentLocationURI =
+ URIUtils.getUri("mockPath", OFFLINE_TABLE_NAME,
URIUtils.encode(segmentMetadata.getName()));
+ File currentSegmentLocation = new File(new File("foo/bar"), "mockChild");
+
+ zkOperator
+ .completeSegmentOperations(OFFLINE_TABLE_NAME, segmentMetadata,
finalSegmentLocationURI,
+ currentSegmentLocation, true, httpHeaders, "downloadUrl",
+ true, "crypter");
+ fail();
+ } catch (Exception e) {
+ // Expected
+ }
+
+ // Add a tiny sleep to give some time for the segment Zk entry to be
deleted.
+ Thread.sleep(5000L);
Review comment:
Done. Thanks!
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/api/upload/ZKOperatorTest.java
##########
@@ -133,4 +158,4 @@ public void testCompleteSegmentOperations()
public void tearDown() {
ControllerTestUtils.cleanup();
}
-}
+}
Review comment:
Done.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
