Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-16405: Remove unneeded errorprone suppressions #1001

Merged
merged 2 commits into from Sep 9, 2022

Conversation

risdenk
Copy link
Contributor

@risdenk risdenk commented Sep 7, 2022

@epugh
Copy link
Contributor

epugh commented Sep 7, 2022

My methodology is super suspect, I checked out main, and looked the java errors in intelliJ, then I checked out this branch, and looked at the java errors in intelliJ. We went from 27,544 warnings to 27,448 warnings. So this has my +1.

@risdenk
Copy link
Contributor Author

risdenk commented Sep 7, 2022

I think this found a potentially real bug in:

TestDistributedStatsComponentCardinality

   >     java.lang.AssertionError: int_i: relativeErr=0.09722718241315029, estimate=4746, real=5337, p=q=id_i1:[4807+TO+10143]&rows=0&stats=true&stats.field={!cardinality%3Dtrue+hllLog2m%3D7+hllRegwidth%3D8}int_i&stats.field={!cardinality%3Dtrue+hllLog2m%3D7+hllRegwidth%3D8+hllPreHashed%3Dtrue}int_i_prehashed_l&stats.field={!cardinality%3Dtrue+hllLog2m%3D7+hllRegwidth%3D8}long_l&stats.field={!cardinality%3Dtrue+hllLog2m%3D7+hllRegwidth%3D8+hllPreHashed%3Dtrue}long_l_prehashed_l&stats.field={!cardinality%3Dtrue+hllLog2m%3D7+hllRegwidth%3D8}string_s&stats.field={!cardinality%3Dtrue+hllLog2m%3D7+hllRegwidth%3D8+hllPreHashed%3Dtrue}string_s_prehashed_l
   >         at __randomizedtesting.SeedInfo.seed([FEAEB9079F935BD9:76FA86DD316F3621]:0)
   >         at org.junit.Assert.fail(Assert.java:89)
   >         at org.junit.Assert.assertTrue(Assert.java:42)
   >         at org.apache.solr.handler.component.TestDistributedStatsComponentCardinality.test(TestDistributedStatsComponentCardinality.java:179)

previously the check was (Math.abs(numMatches - estimate) / numMatches) < relErr where (Math.abs(numMatches - estimate) / numMatches) was integer division so always zero which was less than relErr. The PR changes this to ((float) Math.abs(numMatches - estimate) / numMatches) < relErr which now correctly uses floats. And there are cases where the relErr and calculated error are different.

@risdenk
Copy link
Contributor Author

risdenk commented Sep 7, 2022

I think the above error is actually https://issues.apache.org/jira/browse/SOLR-10918

@risdenk
Copy link
Contributor Author

risdenk commented Sep 7, 2022

I think the above error is actually https://issues.apache.org/jira/browse/SOLR-10918

ehhh it might be related - but running ./gradlew -p solr/core beast -Ptests.dups=10 --tests "org.apache.solr.handler.component.TestDistributedStatsComponentCardinality" still fails when putting that failing assert inside the SOLR-10918 if statement...

@risdenk risdenk force-pushed the SOLR-16405 branch 2 times, most recently from 0108295 to 02cecd6 Compare September 8, 2022 16:01
@risdenk
Copy link
Contributor Author

risdenk commented Sep 8, 2022

So in commit 36fc252, I finally found a log2m value that gives some accurate enough results for testing. I am beasting this test to make sure all is good.

* Increase log2m minimum to get better estimates during testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants