Jackie-Jiang commented on a change in pull request #5221: Add a new server api for download of segments. URL: https://github.com/apache/incubator-pinot/pull/5221#discussion_r408518712
########## File path: pinot-server/src/test/java/org/apache/pinot/server/api/BaseResourceTest.java ########## @@ -58,8 +59,8 @@ protected static final String TABLE_NAME = "testTable"; private final Map<String, TableDataManager> _tableDataManagerMap = new HashMap<>(); - protected final List<ImmutableSegment> _indexSegments = new ArrayList<>(); - + protected final List<ImmutableSegment> _realtimeIndexSegments = new ArrayList<>(); Review comment: I agree the test setup should reflect the true cluster setup, where the table name should have the correct type suffix. My concern about the CONSUMING segment is because the CONSUMING segment is not ImmutableSegment, so I'm not sure if adding separate realtime segments as a list of ImmutableSegment is the correct design. Currently the test setup is mimicking a offline only table, so I would suggest introducing the realtime part as a separate PR (the fix for the table name is good). ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
