yuanbenson commented on code in PR #9295:
URL: https://github.com/apache/pinot/pull/9295#discussion_r958994759


##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/SegmentUploadIntegrationTest.java:
##########
@@ -81,10 +89,15 @@ protected List<String> getBloomFilterColumns() {
     return null;
   }
 
+  @BeforeMethod
+  public void setUpTest()

Review Comment:
   Calling `TestUtils.ensureDirectoriesExistAndEmpty(_tempDir, _segmentDir, 
_tarDir)` in `setUp` is insufficient as there is another test case 
`testUploadAndQuery` that leaves data in the tmp directories.
   
   Using two table names will also lead to more code for now, since it seems 
like `BaseClusterIntegrationTest` is written in a way that assumes one 
table/config per subclass, e.g. `getTableName`, `createOfflineTableConfig`. 
   
   Hence, I prefer to keep the test structure as is with the before and after 
method annotations, unless this is an anti-pattern. Another way is to create a 
different test class altogether, but combining the two here to not prevent slow 
down from too many tests. 



-- 
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

Reply via email to