Code cleanup from query profile project

[paul-rogers]

Description

Code cleanup from query profile project. Cleanup changes done as separate PR to simplify review of the "substantial" changes.

• Fix spelling errors
• Fix Javadoc formatting
• Abstract out repeated test code
• Reuse constants in place of some string literals
• Fix up some parameterized types
• Reduce warnings reported by Eclipse
• Add jenv file and generated "pmd" files to .gitignore

There are no functional changes.

This PR has:

• been self-reviewed.
• been tested in a by running a full local Druid build & test cycle.

[paul-rogers]

Travis build, license check, failed, but seems to be spurious:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

All other Phase 1 tests passed (nor surprising, there are no functionality changes in the PR), but because of the flakey Phase 1 build, Phase 2 didn't run. How to I nudge it to try again, just add a trivial commit? I don't see a "try again" button anywhere...

[FrankChen021]

overall LGTM. CI has been re-triggered.

[jihoonson]

@paul-rogers thanks for adding javadocs. I left some comments about them.

[jihoonson]

👍 thanks for adding javadoc!

[paul-rogers]

@FrankChen021 and @jihoonson, thanks for the review! Addressed your comments. In some cases, it meant reverting the change (this is just a cleanup PR, and leaving things be is sometimes the simplest solution.)

[paul-rogers]

Looks like some conflicts have appeared. Please take a look at the most recent changes. After that, I'll squash commits and resolve conflicts. (It is a pain to resolve conflicts with unsquashed commits.)

[jihoonson]

@paul-rogers thank you for your patience! The latest changes LGTM. +1 after conflicts are resolved.

[paul-rogers]

@jihoonson, thanks much for the review. Resolved merge conflicts. Easiest way was to squash commits and rebase on master. Only the files mentioned in the conflict message above were changed. Did not do a full test run; let's see what the Travis run here tells us.

Edit: argh, the conflict message was removed by this latest push. The files were:

• .gitignore
• core/src/main/java/org/apache/druid/java/util/common/ISE.java
• integration-tests/src/test/java/org/apache/druid/tests/query/ITSqlCancelTest.java
• sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java

[paul-rogers]

Looks like the build is failing due to incomplete tests. In file ParallelMergeCombiningSequence.java, we replaced this line:

  out.offer(ResultBatch.TERMINAL);

With this one:

  out.offer((ParallelMergeCombiningSequence.ResultBatch<T>) ResultBatch.TERMINAL);

Which is clearly only a compile-time change (to remove a warning.) Yet, the build fails with this error:

Diff coverage statistics:
------------------------------------------------------------------------------
|     lines      |    branches    |   functions    |   path
------------------------------------------------------------------------------
|   0% (0/1)     | 100% (0/0)     | 100% (0/0)     | org/apache/druid/java/util/common/guava/ParallelMergeCombiningSequence.java
------------------------------------------------------------------------------
...
ERROR: Insufficient line coverage of 0% (0/1). Required 50%.

This error prevents the Stage 2 tests from running.

It seems we've got a situation where we can't clean up warnings in code until we figure out how to fully unit test the code. While having tests is a Good Thing, it seems we've wrapped ourselves around the axel because we didn't require tests when the code was written, but we do require someone else to write the tests to clean up warnings.

Suggestions?

[jihoonson]

It seems we've got a situation where we can't clean up warnings in code until we figure out how to fully unit test the code. While having tests is a Good Thing, it seems we've wrapped ourselves around the axel because we didn't require tests when the code was written, but we do require someone else to write the tests to clean up warnings.

@paul-rogers thanks for bringing this up. People, including myself, have been struggling with a similar issue from time to time. The coverage bot is good to have because it reduces the burden of code review. The reviewer should manually check test coverage otherwise. For your point, I don't think we can enforce the original author to add all tests that cover 100% of the change because it will be another way to wrap ourselves around the axel 🙂 Making a PR will become extremely hard if we raise the bar of coverage check to 100%. I think our current problem is that the bot is not smart enough to tell what are real changes that impact production code path and what are not. We should work on this to make the bot better over time.

For this particular PR, I think there could be 3 options for us.

• Not fixing the warning (or just suppressing the warning). The warning seems trivial and I don't mind living with it.
• Ignoring the test coverage check failure. This makes sense only when all tests in the phase 2 pass. Travis allows us to manually start those tests, but they should be started one by one by clicking the restart button in Travis. This is doable but there are more than 60 tests in the phase 2 and my wrist will hurt after restarting all of them 🙂
• Adding a new test. I prefer this option most as it's good to have a larger test coverage. It seems not that hard to add a test for this issue if you add a custom fjp pool that always explodes in execute() since it is always called in MergeCombinePartitioningAction.compute(). You may want to make MergeCombinePartitioningAction VisibleForTesting for easy testing.

[jihoonson]

@paul-rogers thanks for updating the PR. I just retriggered the failed test which seems flaky. I will merge this PR after it passes.

[jihoonson]

Merging this PR. Thanks @paul-rogers!

[paul-rogers]

@jihoonson, thanks much for your help!