[
https://issues.apache.org/jira/browse/LUCENE-10143?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17424099#comment-17424099
]
Uwe Schindler edited comment on LUCENE-10143 at 10/4/21, 6:32 PM:
------------------------------------------------------------------
bq. In general, I'm gonna say the "incomplete delegator" case isn't worth
trying to address here. Only the rate limiter is a delegator!
But you allow me to add stuff that i noticed when skimming through the code/PR?
Sorry, but I am a fan of "brainstorming" and sorry that I used this issue here.
It is unfair to tell me "no no complex build logic no no no". Everything is
incremental. When we imvented forbiddenapis you could also say "no no no
complex build logic" - now everybody uses it! So please allow me to investigate
options and discuss them here. The problem in THIS issue was a typical
delegator issue, so allow me to discuss solutions to prevent this.
Some background: The delegator issue is just something I found always
problematic and we have tons of bugs about that (like FilteredTermsEnum,...)
and had a painful experience to find them and fix them. So some build logic to
enfoce the correct "delegator" pattern is worth to investigate. I would really
be happy if plain simple ECJ could warn/fail if you have an incomplete
delegator!
Back to your PR and future related work:
bq. So I agree, let's not make the methods abstract as it makes a lot of noise.
I am fine with working on improving some of the implementations out there, but
my problem was making it abstract and cause code bloat. And as [~jpountz] found
out there are even more places where we can use the new VarHandles. But in
general most cases of writeLong() in custom DataOutputs are perfectly fine and
optimize just fine by hotspot. The problem is only code which adds much
conditional logic in each writeByte(). But those are also a problem with
writeVInt(). IMHO, we should also fix the other writeVInt/writeVLong methods in
RateLimitedIndexOutput by calling in.write....().
was (Author: thetaphi):
bq. In general, I'm gonna say the "incomplete delegator" case isn't worth
trying to address here. Only the rate limiter is a delegator!
But you allow me to add stuff that i noticed when skimming through the code/PR?
Sorry, but I am a fan of "brainstorming" and sorry that I used this issue here.
It is unfair to tell me "no no complex build logic no no no". Everything is
incremental. When we imvented forbiddenapis you could also say "no no no
complex build logic" - now everybody uses it! So please allow me to investigate
options and discuss them here. The problem in THIS issue was a typical
delegator issue, so allow me to discuss solutions to prevent this.
Some background: The delegator issue is just something I found always
problematic and we have tons of bugs about that (like FilteredTermsEnum,...)
and had a painful experience to find them and fix them. So some build logic to
enfoce the correct "delegator" pattern is worth to investigate. I would really
be happy if plain simple ECJ could warn/fail if you have an incomplete
delegator!
Back to your PR and future related work:
bq. So I agree, let's not make the methods abstract as it makes a lot of noise.
I am fine with working on improving some of the implementations out there, but
my problem was making it abstract and cause code bloat. And as [~jpountz] found
out there are even more places where we can use the new VarHandles. But in
general most cases of writeLong() in custom DataOutputs are perfectly fine and
optimize just fine by hotspot. The problem is only code which adds much
conditional logic in each writeByte(). But those are also a problem with
> RateLimitedIndexOutput should delegate writeShort/writeInt/writeLong
> --------------------------------------------------------------------
>
> Key: LUCENE-10143
> URL: https://issues.apache.org/jira/browse/LUCENE-10143
> Project: Lucene - Core
> Issue Type: Improvement
> Reporter: Adrien Grand
> Priority: Minor
> Attachments: screenshot-1.png
>
> Time Spent: 3h 10m
> Remaining Estimate: 0h
>
> Otherwise merges are not taking advantage of LUCENE-10125.
> cc [~uschindler]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
