jihoonson commented on pull request #10336: URL: https://github.com/apache/druid/pull/10336#issuecomment-686045841
> In general, a common question I've asked myself while reviewing this patch is are there any places where we now parse more rows vs failing earlier in the process by directly throwing an exception. It was hard for me to discern that reading this patch. Do you have any suggestions on how to look for cases where an exception is being swallowed instead of being thrown immediately? I mainly touched Kafka/Kinesis tasks and IndexTask and we have some tests for them which verify the metrics in `RowIngestionMeters`. Check out `IndexTaskTest`, `KafkaIndexTaskTest`, and `KinesisIndexTaskTest`. So I believe there is no bug in at least what are covered by those tests. One of the goals in this PR is clarifying where `ParseException` can be thrown. I first tried to change `ParseException` to be a regular `Exception` instead of `RuntimeException`, but it requires to modify all method signatures used on the query side even though `ParseException` will not be thrown from them. Since this doesn't look nice, I decided to scope down the places where can potentially throw `ParseException`s. After this PR, the only possible places are `InputEntityReader` (and all classes it uses) and `IncrementalIndex.addToFacts()` and any `ParseException` thrown in other places is a bug. ---------------------------------------------------------------- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
