gortiz commented on PR #8979: URL: https://github.com/apache/pinot/pull/8979#issuecomment-1198909909
> assert can be enabled by setting a JVM parameter: -ea Yeah, that is the behavior of assert. The problem is: In several parts of Pinot there are try-catch sections that catch Exception and do something (logging, returning something special, etc). Usually it is a bad practice in Java to catch Throwable because most of the times the application can not survive to an Error. For example, if we are getting out of memory or if there is an assertion that fail. But that usual good practice does not always apply. For example I found that by not catching OOM errors in the ingestion jobs we are in fact not failing when one of them is thrown and therefore we break the consistency. In this PR I found a similar problem: As our code is not catching the AssertionError that assert throws, the integration tests were neither failing appropriately neither logging the problem. Instead I've got a very random error and no logging, which makes it very difficult to understand where the problem was. We have two options here: Either modify our codebase to try to catch all the Throwables in the root of the threads or do not use assertions. What we cannot do is to continue using assert when it is more problematic than useful. -- 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
