kbendick commented on PR #4953: URL: https://github.com/apache/iceberg/pull/4953#issuecomment-1146737859
I’m worried about suppressing this rule entirely. Does that mean future classes won’t report this warning when they’re being written? Those classes would then be doing an unnecessary allocation to an array pretty often. If so, I’d prefer suppressing this specific instance if we don’t care to change it. Having looked at the blog briefly, I understand the argument that the JVM hash code truly isn’t consistent across versions etc. But in the general and common case this rule still warns against an unnecessary array allocation, right? It seems strange to globally block that rule over locally suppressing or just saying it’s fine to change. Is there some bigger argument I’m missing? > So what I’d like to ask for is this: if you’re building a distributed framework based on the JVM, please don’t use Java’s hashCode() for anything that needs to work across different processes. Because it’ll look like it works fine when you use it with strings and numbers, and then someday a brave soul will use (e.g.) a protocol buffers object, and then spend days banging their head against a wall trying to figure out why messages are getting sent to the wrong servers. While this very well may be true about Java hashCode and I won’t dispute your argument. But can we confirm that this isn’t being used this way in this library at times and downstream in this way and assumed somewhat deterministic? Not in my opinion. Iceberg is used in many places. I’d prefer to add a local suppression, but if we want to change it then just change it here and keep the warning. It’s good advice to not call `Objects.hash` with only one argument because of the array allocation still. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
