You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/06/23 18:44:32 UTC
[GitHub] [incubator-pinot] kkrugler opened a new issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
kkrugler opened a new issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082
In `SegmentDeletionManager.removeAgedDeletedSegments()`, there’s this bit of code:
``` java
try {
// Check that the directory for deleted segments exists.
if (!pinotFS.isDirectory(deletedDirURI)) {
LOGGER.warn("Deleted segment directory {} does not exist or it is not directory.", deletedDirURI.toString());
return;
}
```
But if we’ve never deleted any segments, then this "Deleted_Segments" directory won’t exist yet. So shouldn’t this check be “if it exists && it’s not a directory”? Otherwise we get regular warnings in our log files, which trigger an alert, etc, etc.
Also the `HadoopPinotFS.isDirectory()` call throws a `FileNotFoundException` if called on a non-existent file/directory, which isn’t caught in this method, bubbles, up, and generates a `Could not get file status for hdfs:///path/to/Deleted_Segments` error.
@mayankshriv said:
> 1. What is the expected behavior wrt Deleted Segments directory? Is it that if directory does not exist then we don't save, or is it that we always save, in which case we should create this directory in ControllerStarter.
> 2. Depending on what's the consensus on 1, we can either assert or make this a debug message after adding Ken's suggested check, instead of warn.
> IMHO, we should not force DeletedSegments directory to be created (unless user specifies in a conf), and make this a debug message, along with exists() check.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082#issuecomment-869916753
@kkrugler @mcvsubbu Fix for the issue, please take a look: #7097
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang closed issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang closed issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082#issuecomment-867118235
I think we should check and create delete segments directory if missing in the constructor of `SegmentDeletionManager`
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kkrugler commented on issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
kkrugler commented on issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082#issuecomment-867238816
Hi @mcvsubbu - ops guy reported warnings every 5 minutes, but looks like that was just the alerting system firing over and over again, not actually due to new entries in the warn/error log file that we set up for each service.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082#issuecomment-867230360
@kkrugler you are right. The two warnings in lines 220 and 226 should not be so. The first one can be removed, and the second one can be turned into an INFO, perhaps. Are you configuring retention manager to run ever 5 mins? The default is 6 hours.
And yes, I am fine with SegmentDeletionManager auto-creating the dit as well. In which case, the first `if` can be removed.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kkrugler edited a comment on issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
kkrugler edited a comment on issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082#issuecomment-867175051
Hi @mcvsubbu
> @kkrugler the call to remove aged deleted segments, is invoked from retention manager. If you never ever delete anything from any table in your cluster, you can (for now) disable RetentionManager. Alternatively, create a test table with a segment and delete it so that the parent dir is created on deletion. This will avoid your warnings and alerts.
Yes, I've manually created the directory, so the warnings and alerts are gone.
> It seems to be that HadoopPinotFS is miscoded. The method isDirectory() is a boolean, and if it does not exist or is not a directory, it should return false. IMO the fix is in the underlying FS implementation.
Agreed, as that would be more consistent with how Java's `File.isDirectory()` works. I've filed a [separate issue](https://github.com/apache/incubator-pinot/issues/7083) for that.
Though even if this method worked as expected, and returned false, we'd get a warning in the logs every 5 minutes or so.
Which is why I agree with @Jackie-Jiang that we should try to auto-create the directory, as then the warning is for a real problem (something went wrong with the creation, or it was accidentally deleted).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang closed issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang closed issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kkrugler commented on issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
kkrugler commented on issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082#issuecomment-867175051
Hi @mcvsubbu
> @kkrugler the call to remove aged deleted segments, is invoked from retention manager. If you never ever delete anything from any table in your cluster, you can (for now) disable RetentionManager. Alternatively, create a test table with a segment and delete it so that the parent dir is created on deletion. This will avoid your warnings and alerts.
Yes, I've manually created the directory, so the warnings and alerts are gone.
> It seems to be that HadoopPinotFS is miscoded. The method isDirectory() is a boolean, and if it does not exist or is not a directory, it should return false. IMO the fix is in the underlying FS implementation.
Agreed, as that would be more consistent with how Java's `File.isDirectory()` works. I'll file a separate issue for that.
Though even if this method worked as expected, and returned false, we'd get a warning in the logs every 5 minutes or so.
Which is why I agree with @Jackie-Jiang that we should try to auto-create the directory, as then the warning is for a real problem (something went wrong with the creation, or it was accidentally deleted).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082#issuecomment-869916753
@kkrugler @mcvsubbu Fix for the issue, please take a look: #7097
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on issue #7082: Fix handling of missing Deleted_Segments directory in SegmentDeletionManager
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on issue #7082:
URL: https://github.com/apache/incubator-pinot/issues/7082#issuecomment-867084996
According to this
https://github.com/apache/incubator-pinot/blob/master/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java#L78
DeletedSegments directory will be created on demand, when the first segment is deleted.
@kkrugler the call to remove aged deleted segments, is invoked from retention manager. If you never ever delete anything from any table in your cluster, you can (for now) disable RetentionManager. Alternatively, create a test table with a segment and delete it so that the parent dir is created on deletion. This will avoid your warnings and alerts.
It seems to be that HadoopPinotFS is miscoded. The method isDirectory() is a boolean, and if it does not exist or is not a directory, it should return false. IMO the fix is in the underlying FS implementation.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org