ctubbsii commented on PR #6457: URL: https://github.com/apache/accumulo/pull/6457#issuecomment-4860451945
I'm abandoning this change. I don't think it's a good idea anymore for the following reasons: 1. This also removes the protective copies from the setters/constructors, which existed prior to #6430 and we didn't have a problem with those. I don't think it's worth removing those along with the ones for the getter. 2. The `.getValue()` method that returns a `byte[]` does not change with this PR, and that's the one that we are now using, not the `.bufferForValue()` method, which is affected by this change. This means that our code isn't really impacted by this change. The bigger impact from #6430 is just the switch from direct access to the internal ByteBuffer to the method that returns a byte array, rather than one that makes a protective copy of the ByteBuffer. The equivalent to before this PR would be to use the `.bufferForValue()` version of methods, but I don't necessarily think that's a good idea either. 3. The method we are using, `.getValue()`, does not actually make copies, except in the case where a copy is needed to truncate the underlying byte buffer's array to size. Other than that, there's just a very lightweight object allocation to wrap with a ByteBuffer interface, which I suspect is not really the cause of any performance issues. 4. I think ultimately, any issues with the RPC layer are really best addressed in calling code, but I think the protective copies and the use of 5. It should be noted that the GarbageCollector IT that started failing after #6430 is already flaky, and sometimes times out even after reverting #6430. While #6430 may have made the situation slightly more reliably fail, I still think that the best fixes are probably in the calling code, and that test probably needs to tuning anyway and should not be considered a reliable indicator that the RPC changes are a problem overall. We can continue the discussion on #6455 for possible next steps. -- 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]
