xiangfu0 commented on code in PR #19925:
URL: https://github.com/apache/pinot/pull/19925#discussion_r4344735217
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/json/OffHeapJsonIndexCreator.java:
##########
@@ -74,7 +74,18 @@ public OffHeapJsonIndexCreator(File indexDir, String
columnName, String tableNam
throws IOException {
super(indexDir, columnName, tableNameWithType, continueOnError,
jsonIndexConfig);
_postingListFile = new File(_tempDir, POSTING_LIST_FILE_NAME);
- _postingListOutputStream = new DataOutputStream(new
BufferedOutputStream(new FileOutputStream(_postingListFile)));
+ try {
Review Comment:
Nit: `catch (Throwable t)` is broad — it'll also catch things like
`OutOfMemoryError`. That's arguably fine for cleanup-and-rethrow, but `catch
(Exception | Error t)` or just `catch (IOException t)` (the only checked type
`FileOutputStream` throws) would be a touch more conventional here. Not
blocking.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/json/OffHeapJsonIndexCreator.java:
##########
@@ -74,7 +74,18 @@ public OffHeapJsonIndexCreator(File indexDir, String
columnName, String tableNam
throws IOException {
super(indexDir, columnName, tableNameWithType, continueOnError,
jsonIndexConfig);
_postingListFile = new File(_tempDir, POSTING_LIST_FILE_NAME);
- _postingListOutputStream = new DataOutputStream(new
BufferedOutputStream(new FileOutputStream(_postingListFile)));
+ try {
+ _postingListOutputStream = new DataOutputStream(new
BufferedOutputStream(new FileOutputStream(_postingListFile)));
+ } catch (Throwable t) {
+ // The base constructor already created the temp dir, but a failure here
leaves the caller without an instance to
+ // close, so the temp dir has to be cleaned up here. Call super.close()
because close() dereferences the stream.
+ try {
Review Comment:
Tiny wording nit: "Call super.close() because close() dereferences the
stream" reads a bit ambiguously. The real point is that `this.close()` (the
override below) would NPE here since the final field is still unassigned, so we
deliberately call the base `super.close()` which only cleans the temp dir.
Maybe "call super.close() instead of this.close(), which would NPE on the
unassigned stream."
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
