ctubbsii commented on PR #81: URL: https://github.com/apache/accumulo-proxy/pull/81#issuecomment-1683259354
Thanks for tracking down the failure, @drewfarris . To add some more context to this, Accumulo didn't use any other protocols for itself, for its own (non-public) RPC, so those extra methods to support using other protocols inside Accumulo's utility code were not being used, and were simplified, because they weren't needed internally, and these were not public APIs. It does appear that the proxy itself allows the protocol to be specified in its own configuration. That's reasonable, since its RPC *is* its API, and consumers of the proxy may have need of using one protocol over another for their application. Since these tests verify that feature of the proxy work, I think the fact they are failing legitimately reveals the fact that this PR is not good enough to merge on its own. I would argue for keeping those ITs. Instead of removing the ITs that test a desirable proxy feature, and conforming the proxy code to what is still accessible inside Accumulo's internal utility code, the proxy should avoid using Accumulo's internal utility code entirely, and start Thrift services using Thrift server boilerplate that exists inside the proxy itself. That will keep a strong separation between Accumulo's internals and the proxy's. -- 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]
