Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

TAJO-1279: Cleanup TajoAsyncDispatcher and interrupt stop events.#331

Closed
jinossy wants to merge 14 commits intoapache:masterfrom
jinossy:TAJO-1279
Closed

TAJO-1279: Cleanup TajoAsyncDispatcher and interrupt stop events.#331
jinossy wants to merge 14 commits intoapache:masterfrom
jinossy:TAJO-1279

Conversation

@jinossy
Copy link
Copy Markdown
Member

@jinossy jinossy commented Jan 6, 2015

No description provided.

@jihoonson
Copy link
Copy Markdown
Contributor

I'm reviewing the patch.

@jihoonson
Copy link
Copy Markdown
Contributor

When I run 'mvn clean install', tests are frequently hanged with the following error.

2015-01-07 21:58:07,530 INFO: org.apache.zookeeper.ClientCnxn (logStartConnect(975)) - Opening socket connection to server xxx.xxx.xxx.xxx/xxx.xxx.xxx.xxx:58986. Will not attempt to authenticate using SASL (unknown error)

It seems that the zookeeper client cannot find the proper server address.
Would you check this, please?

@jinossy
Copy link
Copy Markdown
Member Author

jinossy commented Jan 7, 2015

@jihoonson
Thank you for the review!
Your log looks like security information. Could you show the error log to me ?
In my log, the connection was established successfully

2015-01-07 22:31:46,996 INFO: org.apache.zookeeper.ZooKeeper (<init>(438)) - Initiating client connection, connectString=kimjh.localhost:61503 sessionTimeout=90000 watcher=catalogtracker-on-hconnection-0x3dbc3890, quorum=kimjh.localhost:61503, baseZNode=/hbase
2015-01-07 22:31:46,998 INFO: org.apache.zookeeper.ClientCnxn (logStartConnect(975)) - Opening socket connection to server kimjh.localhost/192.168.101.41:61503. Will not attempt to authenticate using SASL (unknown error)
2015-01-07 22:31:46,998 INFO: org.apache.zookeeper.ClientCnxn (primeConnection(852)) - Socket connection established to kimjh.localhost/192.168.101.41:61503, initiating session
2015-01-07 22:31:46,998 INFO: org.apache.zookeeper.server.NIOServerCnxnFactory (run(197)) - Accepted socket connection from /192.168.101.41:65438

@hyunsik
Copy link
Copy Markdown
Member

hyunsik commented Jan 8, 2015

+1
The patch looks good to me. The unit tests are successfully passed. The patch seems to find unreleased resources.

@jihoonson,

Do you have more stuff that you need to review? If not, I want that this patch is committed to master as soon as possible due to recently frequent CI failures.

@jihoonson
Copy link
Copy Markdown
Contributor

@hyunsik thanks for your review. I also think that we should resolve this issue as soon as possible.

All changes look good to me, too. The only thing I concern is, as commented above, the frequent hangs during unit tests. Whenever a zookeeper client fails to establish a connection, the test waits for 10 ~ 50 seconds. I think that it will significantly bother us.

I cannot find this error in the current master branch. Thus, I suspect that an existing, but hidden error is exposed due to the changed HBase-related configurations. Maybe we can fix it in another issue.

What do you think about it?

@jinossy
Copy link
Copy Markdown
Member Author

jinossy commented Jan 8, 2015

@jihoonson I will remove the HBase-related codes

+    conf.setInt(HConstants.HBASE_CLIENT_IPC_POOL_SIZE, 5);
+    conf.setInt(HConstants.REGION_SERVER_HANDLER_COUNT, 5);
+    conf.setInt(HConstants.REGION_SERVER_META_HANDLER_COUNT, 2);
+    conf.setInt(HConstants.MASTER_HANDLER_COUNT, 5);
+    conf.setInt("hbase.hconnection.threads.max", 5);
+    conf.setInt("hbase.hconnection.threads.core", 5);

@jihoonson
Copy link
Copy Markdown
Contributor

@jinossy thanks. We can commit the patch immediately.
Anyway, we must resolve the zookeeper-related problem in another issue.

@jinossy
Copy link
Copy Markdown
Member Author

jinossy commented Jan 8, 2015

@jihoonson
I don’t think so. Hbase was merge by TAJO-1233. and This patch is not related miniZookeeperCluster binding issue.
Do you still have the problem in this patch?

@jihoonson
Copy link
Copy Markdown
Contributor

@jinossy, I have tested the latest patch. It worked well.
Here is my +1.
Thanks!

@jinossy
Copy link
Copy Markdown
Member Author

jinossy commented Jan 8, 2015

@jihoonson Thanks!
I'll commit it shortly.

@asfgit asfgit closed this in 6582d86 Jan 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants