Julian> Do you propose to ban ’throw new RuntimeException(e)’ in all code or just in tests?
I'm 100.42% sure we should ban new RuntimeException(e) and all the other cases like "new Throwable(e)" in test code. pull/1042 touches test code only, so it is more-or-less simple to reason about. That is why I've put rethrow into TestUtil. Julian> in all code That's the question. I'm inclined (up to 99.42%) to ban it in non-test code as well as the benefits greatly overweight drawbacks. Julian> What should people do in non-test code? My idea is to add Calcite-specfic CalciteThrowables#rethrow and use it all over the place (naming and placement is still to be defined). Julian>Use guava’s ’throw Throwables.propagate’ That should never be used. Even Guava deprecated it. We need sneaky-throws, and Guava hasn't that. Julian>I am not inclined to adopt solutions that would make our overall code quality worse. The quality of the codebase won't suffer, however we would get way better exceptions should something fail in runtime. PS. Fun fact: I've seen multiple production outages caused by OutOfMemory when exception class was trying to put full stacktrace into the message. The resulting message took 500+ megabytes even though the initial exception was just 5-10 kilobytes. Vladimir
