Re: [PR] MINOR: Remove deprecation and exception throw in ProcessorRecordContext#hashcode [kafka]
ableegoldman commented on PR #15508: URL: https://github.com/apache/kafka/pull/15508#issuecomment-1998996811 Well 1 of the 4 JDK builds was aborted for some unknown but the other 3 builds completed with only unrelated test failures. Given this is literally only touching on javadocs and error message strings, I'm going to make a judgement call and say it's probably safe to merge and definitely not the cause of the one build being aborted early Thanks for the extra pair of eyes to unravel this "mystery" for me lol. Should always check the original PR for comments when something doesn't make sense...my bad for being lazy here in the initial PR. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Remove deprecation and exception throw in ProcessorRecordContext#hashcode [kafka]
ableegoldman commented on PR #15508: URL: https://github.com/apache/kafka/pull/15508#issuecomment-1993664255 My official take on this is: `Headers` is a stupid class. Why is it mutable?? Seems very silly to me Though I'm sure there is (or at least _was_) a reason... Anyways, pushed the update with just a docs change instead. Give it a +1 when you have a minute -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Remove deprecation and exception throw in ProcessorRecordContext#hashcode [kafka]
ableegoldman commented on PR #15508: URL: https://github.com/apache/kafka/pull/15508#issuecomment-1993658568 Ah, thanks for the reality check guys -- I always forget about `Headers` being mutable 😞 Glad John deprecated the method way back when since the deprecation warning was how I first noticed this (and I was using it in a collection, albeit indirectly). That said, clearly the javadocs could do with a bit more elaboration on _why_ it's mutable. I'll just make this into a simple docs PR in that case -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Remove deprecation and exception throw in ProcessorRecordContext#hashcode [kafka]
mjsax commented on PR #15508: URL: https://github.com/apache/kafka/pull/15508#issuecomment-1989699015 What Bruno says. Git history if you friend :) https://github.com/apache/kafka/pull/6602/files#r278810522 I think we should keep the code as-is and close this PR w/o merging (or add some more detailed comment to explain why the code is this way better). -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Remove deprecation and exception throw in ProcessorRecordContext#hashcode [kafka]
cadonna commented on PR #15508:
URL: https://github.com/apache/kafka/pull/15508#issuecomment-1988235647
@ableegoldman I had a quick look at the code and theoretically
`ProcessorRecordContext` is mutable because `headers()` returns a reference to
the object of `Headers` stored internally. With
`recordContext.headers().remove("myHeader")` the headers in an instance of
`ProcessorRecordContext` can be modified.
Said that, I have no idea whether this was the reason for the deprecation.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] MINOR: Remove deprecation and exception throw in ProcessorRecordContext#hashcode [kafka]
ableegoldman opened a new pull request, #15508: URL: https://github.com/apache/kafka/pull/15508 Something I noticed while working on a new feature -- this class has deprecated its hashcode and actually goes so far as to throw an exception if/when it's invoked. The deprecation note claims this is because the class is mutable, but I don't actually see this happening anywhere in the code. The _reference_ to this object is modified during processing (by the ProcessorContext), but the object itself has no setters and all fields are private. Given the deprecation was years ago, I'm guessing we simply changed from mutating the actual record context object to mutating the reference stored by the ProcessorContext sometime in the past few years, and forgot to remove this deprecation/exception -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
