PR to cleanup VersionBucket for the UpdateLog
Hi All, I have a PR[1] to remove the "highest" field from VersionBucket (highest doc version of the bucket). By reading the code, I understand that this field is used for an optimization in the case of a distributed update and non-leader checking a doc version for an update. But I wondered about the usefulness of this optimization. I ran the SolrIndexing benchmark after removing this field (and cleaning all the code used to maintain it), and it shows no perf difference. By removing this field (a long) from VersionBucket, we can save 500 MB of RAM per Core, because there are 64K VersionBucket. In addition it removes/simplifies code. I plan to merge the PR soon. Please comment there if you have any concerns. Thanks Bruno [1] https://github.com/apache/solr/pull/2021#issuecomment-1774115830
forbidden-apis + assertThat = insanity
Begin Rant...
$ tail -6 ./gradle/validation/forbidden-apis/junit.junit.all.txt
junit.framework.TestCase @ All classes should derive from LuceneTestCase
junit.framework.Assert @ Use org.junit.Assert
junit.framework.** @ Use org.junit
org.junit.Assert#assertThat(**) @ Use org.hamcrest.MatcherAssert.assertThat()
org.junit.matchers.JUnitMatchers @ Use org.hamcrest.CoreMatchers
org.junit.rules.ExpectedException @ Use Assert.assertThrows
So all test classes should:
- derive from LuceneTestCase (agreed)
- use org.junit.Assert instead of junit.framework.Assert (fair enough)
- use MatcherAssert.assertThat() instead of Assert.assertThat() ...
...ok, i guess? ... except that LuceneTestCase extends Assert, giving us
Assert.assertThat() for free. Kind of anoying to have to go out of our
way to import another class to do the same thing.
But also ... you can't just use...
import static org.hamcrest.MatcherAssert.assertThat;
...because (assuming you're following best practices) you're already
inheriting from LuceneTestCase, so that static import is going to be
unused since it conflicts with the inherited 'assertThat' -- or at least
that's what ecjLintTest tells you ... i'm not 100% certain it's correct,
but either way you can't use it because it will fails the build.
So your only recourse is to use...
import org.hamcrest.MatcherAssert;
...and now instead of lots of nice short 'assertThat(...)' method calls
you have to use the twice as long 'MatcherAssert.assertThat', which is
probably going to make spotless decide your assertions needs to span at
least one extra line (assuming you are using descriptive variable names)
All because we don't want want devs to use a (deprecated) method
that's indirectly inherited from an ancestor of a baseclass we tell them
to use.
Which begs the questions:
- What is so terrible about the impl of Assert.assertThat?
- What evil does it contain that makes it completely unssuitable for use?
Let's go find out what exactly we are asking forbidden-apis to
shield us from...
https://github.com/junit-team/junit4/blob/r4.13.2/src/main/java/org/junit/Assert.java#L962
public static void assertThat(String reason, T actual,
Matcher matcher) {
MatcherAssert.assertThat(reason, actual, matcher);
}
...oh dear lord ... what sweet horror is this??!?!?!!?
Thank heavens we were wise enough to protect ourselves from this
unspeakably dangerous code!
-Hoss
http://www.lucidworks.com/
-
To unsubscribe, e-mail: dev-unsubscr...@solr.apache.org
For additional commands, e-mail: dev-h...@solr.apache.org
Re: forbidden-apis + assertThat = insanity
It came in as part of
https://github.com/apache/solr/pull/947#issuecomment-1279651282
I linked to one of the comments there since this was discussed on the PR.
Basically it removes a ton of deprecated usages. If/when we upgrade Junit
we should hopefully have much less to do.
So to answer your question
> - What is so terrible about the impl of Assert.assertThat?
>
Nothing just that it is deprecated and trying to stay on top of avoiding
the use of deprecated code if we can.
- What evil does it contain that makes it completely unssuitable for use?
>
Again same as above since it is the same question twice.
I'll give you that its not ideal, but we have to draw the line somewhere.
Kevin Risden
On Wed, Oct 25, 2023 at 4:27 PM Chris Hostetter
wrote:
>
> Begin Rant...
>
> $ tail -6 ./gradle/validation/forbidden-apis/junit.junit.all.txt
> junit.framework.TestCase @ All classes should derive from LuceneTestCase
> junit.framework.Assert @ Use org.junit.Assert
> junit.framework.** @ Use org.junit
> org.junit.Assert#assertThat(**) @ Use
> org.hamcrest.MatcherAssert.assertThat()
> org.junit.matchers.JUnitMatchers @ Use org.hamcrest.CoreMatchers
> org.junit.rules.ExpectedException @ Use Assert.assertThrows
>
> So all test classes should:
> - derive from LuceneTestCase (agreed)
> - use org.junit.Assert instead of junit.framework.Assert (fair enough)
> - use MatcherAssert.assertThat() instead of Assert.assertThat() ...
>
> ...ok, i guess? ... except that LuceneTestCase extends Assert, giving us
> Assert.assertThat() for free. Kind of anoying to have to go out of our
> way to import another class to do the same thing.
>
> But also ... you can't just use...
>
> import static org.hamcrest.MatcherAssert.assertThat;
>
> ...because (assuming you're following best practices) you're already
> inheriting from LuceneTestCase, so that static import is going to be
> unused since it conflicts with the inherited 'assertThat' -- or at least
> that's what ecjLintTest tells you ... i'm not 100% certain it's correct,
> but either way you can't use it because it will fails the build.
>
> So your only recourse is to use...
>
> import org.hamcrest.MatcherAssert;
>
> ...and now instead of lots of nice short 'assertThat(...)' method calls
> you have to use the twice as long 'MatcherAssert.assertThat', which is
> probably going to make spotless decide your assertions needs to span at
> least one extra line (assuming you are using descriptive variable names)
>
> All because we don't want want devs to use a (deprecated) method
> that's indirectly inherited from an ancestor of a baseclass we tell them
> to use.
>
> Which begs the questions:
>
> - What is so terrible about the impl of Assert.assertThat?
> - What evil does it contain that makes it completely unssuitable for use?
>
> Let's go find out what exactly we are asking forbidden-apis to
> shield us from...
>
>
> https://github.com/junit-team/junit4/blob/r4.13.2/src/main/java/org/junit/Assert.java#L962
>
> public static void assertThat(String reason, T actual,
> Matcher matcher) {
> MatcherAssert.assertThat(reason, actual, matcher);
> }
>
> ...oh dear lord ... what sweet horror is this??!?!?!!?
>
> Thank heavens we were wise enough to protect ourselves from this
> unspeakably dangerous code!
>
>
>
> -Hoss
> http://www.lucidworks.com/
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@solr.apache.org
> For additional commands, e-mail: dev-h...@solr.apache.org
>
>
Re: forbidden-apis + assertThat = insanity
I agree that having to write "MatcherAssert.assertThat" each time is
tedious and makes my code ugly. So finally I try to avoid this nice
construction. Not satisfying.
Le mer. 25 oct. 2023 à 22:44, Kevin Risden a écrit :
> It came in as part of
> https://github.com/apache/solr/pull/947#issuecomment-1279651282
>
> I linked to one of the comments there since this was discussed on the PR.
> Basically it removes a ton of deprecated usages. If/when we upgrade Junit
> we should hopefully have much less to do.
>
> So to answer your question
>
> > - What is so terrible about the impl of Assert.assertThat?
> >
>
> Nothing just that it is deprecated and trying to stay on top of avoiding
> the use of deprecated code if we can.
>
> - What evil does it contain that makes it completely unssuitable for use?
> >
>
> Again same as above since it is the same question twice.
>
> I'll give you that its not ideal, but we have to draw the line somewhere.
>
> Kevin Risden
>
>
> On Wed, Oct 25, 2023 at 4:27 PM Chris Hostetter
> wrote:
>
> >
> > Begin Rant...
> >
> > $ tail -6 ./gradle/validation/forbidden-apis/junit.junit.all.txt
> > junit.framework.TestCase @ All classes should derive from LuceneTestCase
> > junit.framework.Assert @ Use org.junit.Assert
> > junit.framework.** @ Use org.junit
> > org.junit.Assert#assertThat(**) @ Use
> > org.hamcrest.MatcherAssert.assertThat()
> > org.junit.matchers.JUnitMatchers @ Use org.hamcrest.CoreMatchers
> > org.junit.rules.ExpectedException @ Use Assert.assertThrows
> >
> > So all test classes should:
> > - derive from LuceneTestCase (agreed)
> > - use org.junit.Assert instead of junit.framework.Assert (fair enough)
> > - use MatcherAssert.assertThat() instead of Assert.assertThat() ...
> >
> > ...ok, i guess? ... except that LuceneTestCase extends Assert, giving us
> > Assert.assertThat() for free. Kind of anoying to have to go out of our
> > way to import another class to do the same thing.
> >
> > But also ... you can't just use...
> >
> > import static org.hamcrest.MatcherAssert.assertThat;
> >
> > ...because (assuming you're following best practices) you're already
> > inheriting from LuceneTestCase, so that static import is going to be
> > unused since it conflicts with the inherited 'assertThat' -- or at least
> > that's what ecjLintTest tells you ... i'm not 100% certain it's correct,
> > but either way you can't use it because it will fails the build.
> >
> > So your only recourse is to use...
> >
> > import org.hamcrest.MatcherAssert;
> >
> > ...and now instead of lots of nice short 'assertThat(...)' method calls
> > you have to use the twice as long 'MatcherAssert.assertThat', which is
> > probably going to make spotless decide your assertions needs to span at
> > least one extra line (assuming you are using descriptive variable names)
> >
> > All because we don't want want devs to use a (deprecated) method
> > that's indirectly inherited from an ancestor of a baseclass we tell them
> > to use.
> >
> > Which begs the questions:
> >
> > - What is so terrible about the impl of Assert.assertThat?
> > - What evil does it contain that makes it completely unssuitable for use?
> >
> > Let's go find out what exactly we are asking forbidden-apis to
> > shield us from...
> >
> >
> >
> https://github.com/junit-team/junit4/blob/r4.13.2/src/main/java/org/junit/Assert.java#L962
> >
> > public static void assertThat(String reason, T actual,
> > Matcher matcher) {
> > MatcherAssert.assertThat(reason, actual, matcher);
> > }
> >
> > ...oh dear lord ... what sweet horror is this??!?!?!!?
> >
> > Thank heavens we were wise enough to protect ourselves from this
> > unspeakably dangerous code!
> >
> >
> >
> > -Hoss
> > http://www.lucidworks.com/
> >
> > -
> > To unsubscribe, e-mail: dev-unsubscr...@solr.apache.org
> > For additional commands, e-mail: dev-h...@solr.apache.org
> >
> >
>
Re: How about using JDK 21 in the official docker image?
I agree on being conservative here. But if it turns out to work well, we could consider publishing an additional solr:9.4.0-jre21 tag. That way early adopters have a choice. If I remember correctly, Java 21 has some improvements that can benefit some vector workloads or something, so I see a benefit in getting it out there. We could alternatively opt to push temporary images like this to our own apache/solr docker namespace for folks to try out. Jan > 24. okt. 2023 kl. 18:17 skrev Shawn Heisey : > > On 10/18/2023 10:11 AM, Tomasz Elendt wrote: >> I noticed that JDK 21 LTS was released some time ago. Is there any reason >> why official docker images still use JDK 17? >> I'm asking because I know there are some preview JDK features that Lucene >> utilizes and Solr enables them when it detects a newer version (e.g. >> SOLR-16500). >> Does it make sense to switch now that there is a new LTS version? > > I have no desire to stand in the way of progress, but Java 21 has only been > out for a month. I don't think it's a good idea to rely on a new major > version of *anything* that soon after its release. Test with it, but don't > switch to it. > > I do not think we should be planning on such a major upgrade to the docker > image until Java 21 has been out for a while. I was going to upgrade my Solr > server to Java 21 to try it out since it's not a mission critical install, > but Ubuntu doesn't yet have OpenJDK packages for it. The > eclipse-temurin:21-jre-jammy docker image was pushed 11 days ago. > > My thought on it is to wait until at least the release of Java 22, which will > happen six months after Java 21 was released. > > Thanks, > Shawn > > - > To unsubscribe, e-mail: dev-unsubscr...@solr.apache.org > For additional commands, e-mail: dev-h...@solr.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@solr.apache.org For additional commands, e-mail: dev-h...@solr.apache.org
