You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/02/06 16:47:46 UTC
[GitHub] [druid] capistrant opened a new pull request #9324: Fix failing
Calcite unit tests with existing work-a-round for calcite property setup
capistrant opened a new pull request #9324: Fix failing Calcite unit tests with existing work-a-round for calcite property setup
URL: https://github.com/apache/druid/pull/9324
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
Issue #4909 seems to have popped up again. At least for me. I applied the fix from #5451 liberally to all new Calcite test classes introduced in #9279 to fix on my local. Maybe @nishantmonu51 can speak to if my solve properly mirrors his initial fix.
On my local I can reproduce each time by building the sql module without these changes and then remediate by applying these changes. As I said above, I liberally slapped the class extension on each new class that I identified in Calcite as I was working on a different effort when this popped up after merging master into my branch. Hoping Nishant can quickly identify if my application was overkill and I can remove in un-needed locations.
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
This PR has:
- [X] been self-reviewed.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
<hr>
##### Key changed/added classes in this PR
N/A... Just some small unit test changes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #9324: Fix failing Calcite
unit tests with existing work-a-round for calcite property setup
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #9324: Fix failing Calcite unit tests with existing work-a-round for calcite property setup
URL: https://github.com/apache/druid/pull/9324#issuecomment-583039832
@suneet-s what if you just run `mvn test` on the whole sql module? Your tests aren't actually failing for me but tests later on are. It is like calcite is setting up properties once and if the tests run in a certain order on a certain system, everything gets out of whack. But, that may work just fine on your environment... as in the issue referenced above, Gian seemed to have never seen it while the 2 others involved in the issue communications did see it. So dependent on underlying environment of developer executing tests I guess.
I did some analysis on master over lunch that seemed to have narrowed it down to `CalcitePlannerModuleTest`:
##PASS
* `mvn -Dtest=<test>` with each of the new test classes from #9279
* `mvn -Dtest=DruidCalciteSchemaModuleTest,CalciteQueryTest test`
* `mvn -Dtest=NamedDruidSchemaTest,CalciteQueryTest test`
* `mvn -Dtest=NamedLookupSchemaTest,CalciteQueryTest test`
* `mvn -Dtest=NamedSystemSchemaTest,CalciteQueryTest test`
##FAIL
* `mvn test`
* `mvn -Dtest=CalcitePlannerModuleTest,CalciteQueryTest test`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on issue #9324: Fix failing Calcite unit
tests with existing work-a-round for calcite property setup
Posted by GitBox <gi...@apache.org>.
suneet-s commented on issue #9324: Fix failing Calcite unit tests with existing work-a-round for calcite property setup
URL: https://github.com/apache/druid/pull/9324#issuecomment-583049656
@capistrant Thanks for the repro steps. When I wrote the tests, `mvn test` failed locally for a couple classes where it was complaining about something like this
```
org.apache.calcite.sql.validate.SqlValidatorException: Cannot apply <> to the two different charsets UTF-16LE and ISO-8859-1
```
but it was in a class I didn't write, and they passed on Travis, so I didn't dig in to why they were failing locally. I wonder if we should be unsetting the system properties that `CalciteTestBase` is setting in every test. That way, we can trust that each test is running in isolation.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #9324: Fix failing Calcite
unit tests with existing work-a-round for calcite property setup
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #9324: Fix failing Calcite unit tests with existing work-a-round for calcite property setup
URL: https://github.com/apache/druid/pull/9324#issuecomment-583450739
I created #9333 in case I or someone else is able to circle back on this and see if there is a better way to tackle the problem than the current solution.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #9324: Fix failing Calcite
unit tests with existing work-a-round for calcite property setup
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #9324: Fix failing Calcite unit tests with existing work-a-round for calcite property setup
URL: https://github.com/apache/druid/pull/9324
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #9324: Fix failing Calcite
unit tests with existing work-a-round for calcite property setup
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #9324: Fix failing Calcite unit tests with existing work-a-round for calcite property setup
URL: https://github.com/apache/druid/pull/9324#issuecomment-583135063
@suneet-s
I looked into this a little bit and I think I'm okay with Nishants approach to fix this with the `CalciteTestBase` class. It makes sense why he did it, since once the Calcite classes get loaded up and read the calcite specific System properties for configuration, they are stuck like that for the life of the JVM. This was an easy solve to make sure that the system properties were correct for the life of the JVM. It is not obvious to developers to add this unless they reference other tests that touch Calcite that they should extend his base class, which sucks. I'm not sure there is a way to configure Calcite without using System properties and Calcites.java seems to indicate the same in a from the comments in Calcites.setSystemProperties().
TravisCI passing despite all of this has me scratching my head. Maybe the tests are running in a different order than on local..? Or someone in the past messed with the configuration to get past this?
I personally don't see the current approach as a problem. It seems like more of an inconvenience. However, if you have thoughts on a better solution, I think anyone would welcome it because what we currently have here stinks
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on issue #9324: Fix failing Calcite unit
tests with existing work-a-round for calcite property setup
Posted by GitBox <gi...@apache.org>.
suneet-s commented on issue #9324: Fix failing Calcite unit tests with existing work-a-round for calcite property setup
URL: https://github.com/apache/druid/pull/9324#issuecomment-583137257
@capistrant That makes sense. I tried for a little bit to find a cleaner solution, but haven't found a working solution yet.
It looks like the CI failures are flaky. Maybe one of the committers can re-trigger the failed tests.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org