Improve exception message in AbstractService

[leesf]

Fix typo in AbstractService.

[leesf]

@julianhyde And cloud you please review this pr when you have time?

[zabetak]

I am not a big fan of default + exception pattern for the same reasons mentioned in Don't default that switch. All IDEs (for sure Elciplse and Intellij) are capable of detecting easily such cases and return an error or warning depending on the configuration. How about killing it completely?

[julianhyde]

I disagree. That video is talking about a language that (objective c?) that can check that switch statements are exhaustive. I wish that Java did this, but it does not.

In Java, an error that occurs during testing is a good result. Even an error that occurs in production is not too bad - it generates a clear stack trace, which is better than giving mysterious behavior.

If people don't add a default: throw ..., the compiler makes them do silly stuff like 'return null;' after the switch. So it looks as if the method can return a nullable value, even though it cannot in practice. So, default: throw ... is the way to get the best mileage out of javac's (limited) flow analysis.

[leesf]

I agree with julian. Since in Java switch default + exception is very usual in practice, and we just need to give right info in exception stack.

[zabetak]

I was talking exclusively for the switch on enumerations. I am also in favor of fail-fast behavior so that's why I find IDE (or checkstyle) errors/warnings more immediate. Other minor point is that not having a default case means also less code to review and maintain. Having said that both coding styles are valid so if both of you prefer default + exception then that's totally fine by me.

[vlsi]

Just my 2c: @leesf , would you please care to add input getSerializationType() as long as you modify the line?

Then failure message would contain something to analyze rather than obscure Unhandled case statement

[zabetak]

I was going to suggest the same thing, right now!

[zabetak]

Also as a commit message you can use something like "Improve exception message in AbstractService". Avoid the [hotfix] prefix which it is not used in our codebase.

[zabetak]

Please rebase and squash to a single commit! Thanks :)

[leesf]

Thanks for reviewing, @vlsi @zabetak. Updated this PR.

[julianhyde]

I can't believe how much time we've wasted on this PR. The switch is exhaustive. The throw will never happen. So it doesn't matter what the text is. It's a cosmetic change and not worth the effort.

[zabetak]

I don't believe it is a waste of time. There a was a fruitful discussion on if we want to have default case in enumerations in general and I learned that the policy of the project is to always pair default with throws. In general, I think the throw can happen (it should be very very rare) if you happen to mix new and old binaries (classes).

[vlsi]

@zabetak , I'm afraid I'm with Julian.
Each message generates GitHub notification, and it does create disruptions. It forces everybody to spend time on opening the discussion.

As you can see, Julian even navigated here and created a comment. Do you think it is the first time he "reviewed this PR"?

Of course the PR is trivial, and in my opinion committer should just commit the change and that's it.

"Please rebase and squash" generates extreme amount of notifications (see https://github.com/apache/calcite-avatica/watchers ) which is really sad.

That is why I prefer to just rebase/squash/etc on my own when I commit PRs.

There a was a fruitful discussion on if we want to have default case in enumerations

Those cases should probably be discussed on the mailing list.

PS. Really sorry to pile up the notifications.

[julianhyde]

I agree with both of you. @zabetak I'm glad the discussion was useful, in an ideal world it would have happened somewhere else. As @vlsi says, an expedited review process, especially for less important/dangerous PRs, is useful.

An Apache principle is "JFDI" (just *** do it). Which means trust that everyone is acting in good faith, and choose the action which is most efficient. I, too, will squash, rebase and fix up commits as I am reviewing them. (I often need to add author name and the 'close #' comment.) I encourage others to do the same.