Copilot commented on code in PR #17070:
URL: https://github.com/apache/pinot/pull/17070#discussion_r2467868595
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##########
@@ -1164,94 +1182,92 @@ protected SegmentBuildDescriptor
buildSegmentInternal(boolean forCommit)
final long lockAcquireTimeMillis = now();
// Build a segment from in-memory rows.
// If build compressed archive is true, then build the tar.compressed
file as well
- // TODO Use an auto-closeable object to delete temp resources.
+ // Create an auto-closeable object to ensure temp resources are deleted
File tempSegmentFolder = new File(_resourceTmpDir, "tmp-" +
_segmentNameStr + "-" + now());
Review Comment:
The TempFolderCloseable is initialized with tempSegmentFolder before it is
created. If the folder doesn't exist when close() is called,
FileUtils.deleteQuietly() will silently do nothing, which is acceptable.
However, the resource is created on line 1186 but may not be used if an
exception occurs before converter.build() is called, potentially leaving the
folder uncleaned. Consider creating the folder inside the try-with-resources
block or ensuring it exists before wrapping it.
```suggestion
File tempSegmentFolder = new File(_resourceTmpDir, "tmp-" +
_segmentNameStr + "-" + now());
tempSegmentFolder.mkdirs();
```
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##########
@@ -1164,94 +1182,92 @@ protected SegmentBuildDescriptor
buildSegmentInternal(boolean forCommit)
final long lockAcquireTimeMillis = now();
// Build a segment from in-memory rows.
// If build compressed archive is true, then build the tar.compressed
file as well
- // TODO Use an auto-closeable object to delete temp resources.
+ // Create an auto-closeable object to ensure temp resources are deleted
File tempSegmentFolder = new File(_resourceTmpDir, "tmp-" +
_segmentNameStr + "-" + now());
-
- SegmentZKPropsConfig segmentZKPropsConfig = new SegmentZKPropsConfig();
- segmentZKPropsConfig.setStartOffset(_segmentZKMetadata.getStartOffset());
- segmentZKPropsConfig.setEndOffset(_currentOffset.toString());
- // let's convert the segment now
- RealtimeSegmentConverter converter =
- new RealtimeSegmentConverter(_realtimeSegment, segmentZKPropsConfig,
tempSegmentFolder.getAbsolutePath(),
- _schema, _tableNameWithType, _tableConfig,
_segmentZKMetadata.getSegmentName(),
- _defaultNullHandlingEnabled);
- _segmentLogger.info("Trying to build segment");
- try {
- converter.build(_segmentVersion, _serverMetrics);
- } catch (Exception e) {
- String errorMessage = "Could not build segment";
- FileUtils.deleteQuietly(tempSegmentFolder);
- if (e instanceof IllegalStateException || e instanceof
BufferOverflowException) {
- // Index or forward index too large issue, the segment build would
fail consistently
- _segmentBuildFailedWithDeterministicError = true;
+ try (TempFolderCloseable tempFolderCloseable = new
TempFolderCloseable(tempSegmentFolder)) {
+ SegmentZKPropsConfig segmentZKPropsConfig = new SegmentZKPropsConfig();
+
segmentZKPropsConfig.setStartOffset(_segmentZKMetadata.getStartOffset());
+ segmentZKPropsConfig.setEndOffset(_currentOffset.toString());
+ // let's convert the segment now
+ RealtimeSegmentConverter converter =
+ new RealtimeSegmentConverter(_realtimeSegment,
segmentZKPropsConfig, tempSegmentFolder.getAbsolutePath(),
+ _schema, _tableNameWithType, _tableConfig,
_segmentZKMetadata.getSegmentName(),
+ _defaultNullHandlingEnabled);
+ _segmentLogger.info("Trying to build segment");
+ try {
+ converter.build(_segmentVersion, _serverMetrics);
+ } catch (Exception e) {
+ String errorMessage = "Could not build segment";
+ if (e instanceof IllegalStateException || e instanceof
BufferOverflowException) {
+ // Index or forward index too large issue, the segment build would
fail consistently
+ _segmentBuildFailedWithDeterministicError = true;
+ }
+ reportSegmentBuildFailure(errorMessage, e);
+ throw new SegmentBuildFailureException(errorMessage, e);
}
- reportSegmentBuildFailure(errorMessage, e);
- throw new SegmentBuildFailureException(errorMessage, e);
- }
- final long buildTimeMillis = now() - lockAcquireTimeMillis;
- final long waitTimeMillis = lockAcquireTimeMillis - startTimeMillis;
- _segmentLogger.info("Successfully built segment (Column Mode: {}) in {}
ms, after lockWaitTime {} ms",
- converter.isColumnMajorEnabled(), buildTimeMillis, waitTimeMillis);
-
- File dataDir = new File(_resourceDataDir);
- File indexDir = new File(dataDir, _segmentNameStr);
- FileUtils.deleteQuietly(indexDir);
-
- File[] tempFiles = tempSegmentFolder.listFiles();
- assert tempFiles != null;
- File tempIndexDir = tempFiles[0];
- try {
- FileUtils.moveDirectory(tempIndexDir, indexDir);
- } catch (IOException e) {
- String errorMessage = "Caught exception while moving index directory
from: " + tempIndexDir + " to: "
- + indexDir;
- reportSegmentBuildFailure(errorMessage, e);
- throw new SegmentBuildFailureException(errorMessage, e);
- } finally {
- FileUtils.deleteQuietly(tempSegmentFolder);
- }
-
- long segmentSizeBytes = FileUtils.sizeOfDirectory(indexDir);
- _serverMetrics.setValueOfTableGauge(_clientId,
ServerGauge.LAST_REALTIME_SEGMENT_CREATION_DURATION_SECONDS,
- TimeUnit.MILLISECONDS.toSeconds(buildTimeMillis));
- _serverMetrics.setValueOfTableGauge(_clientId,
ServerGauge.LAST_REALTIME_SEGMENT_CREATION_WAIT_TIME_SECONDS,
- TimeUnit.MILLISECONDS.toSeconds(waitTimeMillis));
-
- if (forCommit) {
- File segmentTarFile = new File(dataDir, _segmentNameStr +
TarCompressionUtils.TAR_COMPRESSED_FILE_EXTENSION);
+ final long buildTimeMillis = now() - lockAcquireTimeMillis;
+ final long waitTimeMillis = lockAcquireTimeMillis - startTimeMillis;
+ _segmentLogger.info("Successfully built segment (Column Mode: {}) in
{} ms, after lockWaitTime {} ms",
+ converter.isColumnMajorEnabled(), buildTimeMillis, waitTimeMillis);
+
+ File dataDir = new File(_resourceDataDir);
+ File indexDir = new File(dataDir, _segmentNameStr);
+ FileUtils.deleteQuietly(indexDir);
+
+ File[] tempFiles = tempSegmentFolder.listFiles();
+ assert tempFiles != null;
+ File tempIndexDir = tempFiles[0];
try {
- TarCompressionUtils.createCompressedTarFile(indexDir,
segmentTarFile);
+ FileUtils.moveDirectory(tempIndexDir, indexDir);
} catch (IOException e) {
- String errorMessage = "Caught exception while taring index directory
from: " + indexDir + " to: "
- + segmentTarFile;
+ String errorMessage = "Caught exception while moving index directory
from: " + tempIndexDir + " to: "
+ + indexDir;
reportSegmentBuildFailure(errorMessage, e);
throw new SegmentBuildFailureException(errorMessage, e);
}
- File metadataFile = SegmentDirectoryPaths.findMetadataFile(indexDir);
- if (metadataFile == null) {
- String errorMessage = "Failed to find file: " +
V1Constants.MetadataKeys.METADATA_FILE_NAME
- + " under index directory: " + indexDir;
- reportSegmentBuildFailure(errorMessage, null);
- throw new SegmentBuildFailureException(errorMessage);
- }
- File creationMetaFile =
SegmentDirectoryPaths.findCreationMetaFile(indexDir);
- if (creationMetaFile == null) {
- String errorMessage = "Failed to find file: " +
V1Constants.SEGMENT_CREATION_META + " under index directory: "
- + indexDir;
- reportSegmentBuildFailure(errorMessage, null);
- throw new SegmentBuildFailureException(errorMessage);
- }
- Map<String, File> metadataFiles = new HashMap<>();
- metadataFiles.put(V1Constants.MetadataKeys.METADATA_FILE_NAME,
metadataFile);
- metadataFiles.put(V1Constants.SEGMENT_CREATION_META, creationMetaFile);
+ long segmentSizeBytes = FileUtils.sizeOfDirectory(indexDir);
+ _serverMetrics.setValueOfTableGauge(_clientId,
ServerGauge.LAST_REALTIME_SEGMENT_CREATION_DURATION_SECONDS,
+ TimeUnit.MILLISECONDS.toSeconds(buildTimeMillis));
+ _serverMetrics.setValueOfTableGauge(_clientId,
ServerGauge.LAST_REALTIME_SEGMENT_CREATION_WAIT_TIME_SECONDS,
+ TimeUnit.MILLISECONDS.toSeconds(waitTimeMillis));
+
+ if (forCommit) {
+ File segmentTarFile = new File(dataDir, _segmentNameStr +
TarCompressionUtils.TAR_COMPRESSED_FILE_EXTENSION);
+ try {
+ TarCompressionUtils.createCompressedTarFile(indexDir,
segmentTarFile);
+ } catch (IOException e) {
+ String errorMessage = "Caught exception while taring index
directory from: " + indexDir + " to: "
+ + segmentTarFile;
+ reportSegmentBuildFailure(errorMessage, e);
+ throw new SegmentBuildFailureException(errorMessage, e);
+ }
- return new SegmentBuildDescriptor(segmentTarFile, metadataFiles,
_currentOffset, buildTimeMillis,
- waitTimeMillis, segmentSizeBytes);
- } else {
- return new SegmentBuildDescriptor(null, null, _currentOffset,
buildTimeMillis, waitTimeMillis,
- segmentSizeBytes);
- }
+ File metadataFile = SegmentDirectoryPaths.findMetadataFile(indexDir);
+ if (metadataFile == null) {
+ String errorMessage = "Failed to find file: " +
V1Constants.MetadataKeys.METADATA_FILE_NAME
+ + " under index directory: " + indexDir;
+ reportSegmentBuildFailure(errorMessage, null);
+ throw new SegmentBuildFailureException(errorMessage);
+ }
+ File creationMetaFile =
SegmentDirectoryPaths.findCreationMetaFile(indexDir);
+ if (creationMetaFile == null) {
+ String errorMessage = "Failed to find file: " +
V1Constants.SEGMENT_CREATION_META + " under index directory: "
+ + indexDir;
+ reportSegmentBuildFailure(errorMessage, null);
+ throw new SegmentBuildFailureException(errorMessage);
+ }
+ Map<String, File> metadataFiles = new HashMap<>();
+ metadataFiles.put(V1Constants.MetadataKeys.METADATA_FILE_NAME,
metadataFile);
+ metadataFiles.put(V1Constants.SEGMENT_CREATION_META,
creationMetaFile);
+
+ return new SegmentBuildDescriptor(segmentTarFile, metadataFiles,
_currentOffset, buildTimeMillis,
+ waitTimeMillis, segmentSizeBytes);
+ } else {
+ return new SegmentBuildDescriptor(null, null, _currentOffset,
buildTimeMillis, waitTimeMillis,
+ segmentSizeBytes);
+ }
+ } // End of try-with-resources for TempFolderCloseable
Review Comment:
[nitpick] The comment is redundant and adds noise. The try-with-resources
structure is self-explanatory from the code itself.
```suggestion
}
```
--
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]
