clintropolis commented on PR #13361: URL: https://github.com/apache/druid/pull/13361#issuecomment-1318173059
>Also, there are assertions in the Memory methods as well but since they aren't enabled in deployments, hence we don't get to use them. Maybe, the Memory object in future could itself expose safe wrapping methods which internally runs those assertions as code validations. WDYT? This isn't really up to me, but we have offered to contribute if there is wider use beyond Druids needs, relevant slack thread https://the-asf.slack.com/archives/CP0930GKG/p1666290716186309. > Maybe, the Memory object in future could itself expose safe wrapping methods which internally runs those assertions as code validations. WDYT? I guess I just used a `ByteBuffer` for this because I think such checks largely would negate the reasons of using unsafe, though I guess it would still allow larger memory objects than a java `ByteBuffer` allows to be done in a 'safe' manner if instead I was wrapping another `Memory` object, so maybe that is a better way if we were to merge this functionality in the upstream library since it would be a bit more flexible. I was mostly just being lazy as possible so I just used `ByteBuffer` since the checking was "free" to me. Is that what you meant? >Regarding the compatibility, I think since the method has not been released yet (is it released in 24.0.1?) the default implementation could also throw UOE, but it would mean that all complex types would have to implement it. My personal feelings are that this is at least safe as of this PR to have a default implementation with all core and contrib extensions. Extension implementors have to go out of their way to use `Unsafe` or something like it since they are provided with a `ByteBuffer`, so I think as long as we advise in the release notes and javadocs that any extensions that do this should implement the new `fromByteBufferSafe` method then it is ok. But otoh, it wouldn't be very hard to fill in all the implementations with what I made the default if people feel strongly the other way to force implementors to adapt. > Further, the function could be put behind a cluster level feature flag (not a good thing as well) so that the users/admins are aware of the scope. I'd rather avoid such a flag if possible, the PR is intended to prevent having to have something like that. And as mentioned, it isn't strictly a problem with `COMPLEX_DECODE_BASE64`, such a flag would also need to apply to all ingest time aggregators that read from `byte[]` and map it into `Memory`, which seems complicated. It would definitely be not needed if we did not provide a default implementation of `fromByteBufferSafe`. If it comes down to it, I would much rather drop the default implementation than add the flag. -- 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]
