-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-27278 Improve TestTlsIPC to reuse existing IPC test code #4682
Conversation
This is what I got so far, the bad news is, there are several failures, such as testAsyncEcho...
Seems like multi threading problems, as in async mode we can send multiple request at once. Will dig more. |
@anmolnar @bbeaudreault FYI. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
public void testAsyncEcho() throws IOException {
Configuration conf = HBaseConfiguration.create();
RpcServer rpcServer = createRpcServer(null, "testRpcServer",
Lists.newArrayList(new RpcServer.BlockingServiceAndInterface(SERVICE, null)),
new InetSocketAddress("localhost", 0), CONF, new FifoRpcScheduler(CONF, 1));
try (AbstractRpcClient<?> client = createRpcClient(conf)) { Looks like client TLS never gets enabled in this test. If I change "conf" to "CONF", the test will pass. |
I can make the other 2 tests passing by deriving the client config from CONF. Though I'm not sure, if this is the right thing to do as creating new config for the client might be intentional. So, the other thing we can do is to enable client TLS separately for these config instances. |
/** | ||
* Will cache X509TestContext to speed up tests. | ||
*/ | ||
public class X509TestContextProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you copy this file from ZooKeeper codebase by any chance? We have exactly the same cache for the same purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I implemented it by my own. Glad to see that people have the same thoughts :)
OK, the failure is just a test issue. Updated the PR. |
Yes, this is the problem. I change the tests to create a new configuration based on the existing one, not a fresh new one. |
I also changed the tests to be more 'HBase style'. For example, rename 'hbaseConf' to 'conf' since we are already in the hbase project. And also, use HBaseCommonTestingUtil to create/remove testing directories, etc. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Seems a pre commit problem... I've already removed TestTlsIPC in the patch but when compiling we still reference it... |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@busbey mentioned some weird behaviour with Yetus about an entire test file cannot be removed in a single patch. You have to mark it with Ignore first and remove it in an upcoming PR. |
Seems OK now. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@anmolnar PTAL? Thanks. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks like a good testing improvement/unification. Thanks!
Thanks @bbeaudreault ! |
Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3309108)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Sorry for the late review, I was on vacation.
…e#4682) Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3309108)
…e#4682) Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3309108)
…e#4682) Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3309108)
* HBASE-27185 Rewrite NettyRpcServer to decode rpc request with netty handler (apache#4624) * HBASE-27185 Addendum fix TestShadeSaslAuthenticationProvider * HBASE-27271 BufferCallBeforeInitHandler should ignore the flush request (apache#4676) * HBASE-26666 Add native TLS encryption support to RPC server/client (apache#4666) * HBASE-27278 Improve TestTlsIPC to reuse existing IPC test code (apache#4682) * HBASE-27279 Make SslHandler work with SaslWrapHandler/SaslUnwrapHandler (apache#4705) * HBASE-27342 Use Hadoop Credentials API to retrieve passwords of TLS key/trust stores (apache#4751) * HBASE-27346 Autodetect key/truststore file type from file extension (apache#4757) * HBASE-27280 Add mutual authentication support to TLS (apache#4796) * HBASE-27673 Fix mTLS client hostname verification (apache#5066) * HBASE-27347 Port FileWatcher from ZK to autodetect keystore/truststore changes in TLS connections (branch-2) (apache#4897) * HBASE-27779 Make X509Util config constants public * HBASE-27578 Upgrade hbase-thirdparty to 4.1.4 (apache#4985)
…e#4682) Change[2/4] for: Backporting the changes related to HBASE-27278 (related to TLS). Signed-off-by: Bryan Beaudreault <[email protected]> (cherry picked from commit 3309108) Change-Id: I97c6713059a6e41a56fc0a2614800d251f6349c5
No description provided.