Thank you everyone for your feedback -- I appreciate!
I'll combine it into a small doc and post it for review.
Best regards,
Alexey
On 7/11/17 1:32 PM, Todd Lipcon wrote:
My intuition matches what Dan wrote above. Reproducing what I wrote in
Alexey's gerrit review yesterday:
I dislike Preconditions [for internal invariant checks] because the type of
exception thrown is "IllegalStateException" which is supposed to indicate
to a caller that they called a method at an inappropriate time. In my mind
it's meant to indicate "misuse" of an API, rather than
"mis-programming/bug" of an API (which an AssertionError more clearly
represents).
Again to make the comparison to our C++ style, we use CHECK/DCHECK for
cases where it would be an internal bug to see it fail, and "if (...)
return Status::IllegalArgument(...)" or whatever when it would just be
misuse of an API.
I like how Dan distinguished between "local context" and external usage of
a class. Taking a hypothetical example of a Stack implementation which
maintains its size as an internal integer, I would write something like:
Object pop() {
// Use Preconditions here because the external user of the class should
not call pop()
// on an empty stack, but the stack itself is internally consistent
Preconditions.checkState(curSize > 0, "queue must not be empty");
Object toReturn = data[--curSize];
// Use an assert here because if we ended up with a negative size
counter, that indicates
// that our stack implementation itself is broken - ie it's an invariant,
not a state check.
assert curSize >= 0;
return toReturn;
}
I see Alexey's point, though, that there are some checks that are worth
running even in "production" scenarios, because continuing on if the state
had gone bad would make no sense (or might result in wrong data results,
etc). In those cases I think an 'if (...) throw AssertionError()' could
make sense, and we could hypothetically wrap such a check in a helper class
like 'AlwaysAssert.assertTrue(...)' or whatever.
-Todd
On Tue, Jul 11, 2017 at 1:24 PM, Dan Burkert <[email protected]> wrote:
My general rule of thumb is that I use 'assert' whenever I'm claiming that
something must be true given static (non-runtime) knowledge local to the
context (usually a method or class). If the assert fails, it indicates
that there is irrefutably a bug in that context. I use the Preconditions
helpers for things that should be true, but might be outside the control of
a local context, or might be runtime-dependent. In extreme situations I
might use an explicit if/throw new AssertionException if the consequences
of a hypothetical bug are so extreme that it doesn't make sense to ever
disable the assert (e.g. in a fireMissiles method).
In terms of assert/Preconditions vs DCHECK/CHECK: I think it's not exactly
a 1:1 comparison. We quite frequently return error Status's for things
that we use Preconditions for in Java (illegal argument, illegal state). I
think that's appropriate, given the different error propagation paradigms
of Java vs. our flavor of C++.
Sorry if this doesn't actually suggest a resolution, I think it's a 'know
it when you see it' situation - both Preconditions and asserts have
appropriate uses.
- Dan
On Tue, Jul 11, 2017 at 12:49 PM, Alexey Serbin <[email protected]>
wrote:
Hi,
While working on a small refactoring in the Kudu Java client, I found
that
sometimes we use asserts
and sometimes Preconditions [2] from the guava library to assert on the
consistency of the code.
As Todd suggested, I'm starting this thread to clarify on what is the
best
way to do perform
the consistency checks in our Kudu Java client code. Ideally, the
desired
outcome from the
anticipated discussion would be some sort of guidance on assertion of the
consistency constrains
and invariants in our Java client code. Once we have the guidance, we
could put it as an
additional section of our 'Contributing' page.
The issue with the Java asserts is that they are not applicable unless
the
JVM is run with '-ea'
flag, so it's not possible to explicitly stop processing further
instructions in the context
when an inconsistency is detected.
To me, we have more clarity with the consistency checks and invariant
assertion in our C++ code.
We have CHECK- and DCHECK- derived macros to perform various types of
consistency checks. In short,
the CHECK-related are always there, and the DCHECK-related ones are
present only in debug builds.
We don't turn on/off any of those dynamically in runtime. Also, those
C++
asserts are kind of 'hard'
ones -- the whole program is terminated and you don't need to think how
to
deal with the mess which
isn't possible to recover from. The latter one is a luxury which is not
available in Java
(correct me if I'm wrong).
Putting the explicit termination of the whole program (with an optional
coredump) aside,
the Java's assert looks like DCHECK() but with the ability to turn it
on/off using the run-time switch.
Using guava's Preconditions gives us an option to address that and have a
way
to assert the consistency in our code even if code is run without the
'-ea' JVM flag.
So, to me it looks natural to use the Preconditions-derived checks for
asserting on invariants
even while running in production environment, while using asserts only in
debug/test mode,
asserting on programmatic errors and other non-crucial invariants which
might be handled
by the code even if those checks are removed.
However, this simple approach contradicts to some points of [1] (which,
by
my understanding,
is contradictory and very confusing as is). Also, I'm not a Java
programmer,
so I might be missing some crucial points here.
What do you think? Your feedback is highly appreciated.
Thanks,
Alexey
References:
[1] http://docs.oracle.com/javase/8/docs/technotes/guides/langua
ge/assert.html
[2] https://github.com/google/guava/wiki/PreconditionsExplained
