[STORM-2713] when the connection to the first zkserver is timeout,storm-kafka's kafkaspout will throw a exception by liu-zhaokun · Pull Request #2297 · apache/storm

liu-zhaokun · 2017-08-30T01:57:21Z

https://issues.apache.org/jira/browse/STORM-2713
when the connection to the first zkserver is timeout,storm-kafka's kafkaspout will throw a exception without attempting to connect other zkserver,even zk can also work with one node down.

…rm-kafka's kafkaspout will throw a exception

HeartSaVioR · 2017-08-31T03:23:18Z

 return children.size();
 } catch (Exception e) {
-throw new RuntimeException(e);
+LOG.error(e.getMessage(), e);


Which value do you think this method returns when exception is caught, which action caller should take? Without proper action for failing back, it is better to throw exception and fail-fast.

And compilation fails on the change. Please see the result of Travis build.

@HeartSaVioR
When exception is caught,we should only print the error messages,so that users can be aware of there are some problems in their cluster such as one role of zk was down.

@HeartSaVioR
For example,one role of zk is down in users cluster,storm-kafka will throw a exception,even storm also can connect another zk role and works.I will check the result of Travis build.

When RuntimeException is propagated to call hierarchy, some place will leave a error message. (it is guaranteed when worker is crashing)
Btw, I guess you may need to think and try to answer my questions above by yourself.
Please note that core principal of Storm is 'fail-fast', because in many case things are not restorable and retryable, and we had to kill the process to prevent process to go with unexpected behavior.

That may be viable and possible, but you may need to add retry logic rather than just removing throw statement.

I think it doesn't affect 'fail-fast',I just want to guarantee users' Topology can work normally.

@HeartSaVioR
I mean topology should work even we can't connect one of the zkservers,because the data is shared among zkservers. But if we throw a exception,storm will not attempt to connect other zkserver,and exit.

srdo · 2017-09-19T20:34:31Z

I think Curator is supposed to handle retry and partial connection loss to the Zookeeper cluster for us. I'd rather we use Curator for this instead of trying to add our own retry loops around it.

liu-zhaokun · 2017-09-21T02:42:09Z

@srdo
I don't think it's related to retry, the key is how storm will do when the first zkserver is down.

srdo · 2017-09-21T16:17:12Z

@liu-zhaokun Yes, that's what I'm talking about. If the first zkserver is down, I think Curator is supposed to try another server in the connection string.

liu-zhaokun · 2017-09-23T03:21:09Z

@srdo
I guess you support my PR,right?

srdo · 2017-09-23T09:50:08Z

@liu-zhaokun No, not as it is currently. I think Curator already handles failing over to another Zookeeper server for us, so if the user passes a zkStr with multiple servers here https://github.com/apache/storm/blob/master/external/storm-kafka/src/jvm/org/apache/storm/kafka/DynamicBrokersReader.java#L61 and sets the retry policy at https://github.com/apache/storm/blob/master/external/storm-kafka/src/jvm/org/apache/storm/kafka/DynamicBrokersReader.java#L64 to a few attempts, then I don't see why we need to add another layer of retries on top of that?

Plus as @HeartSaVioR points out, this doesn't compile.

liu-zhaokun · 2017-09-27T02:31:09Z

@srdo
Thanks for your reply.I mean if curator has retried several times and failed,the code where I modify will throw a exception,and it will never attempt to connect another zkserver. Could you help me solve doubts?
I will check the CI later.

We are closing stale Pull Requests to make the list more manageable. Please re-open any Pull Request that has been closed in error. Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469

[STORM-2713] when the connection to the first zkserver is timeout,sto…
aa46372
…rm-kafka's kafkaspout will throw a exception

HeartSaVioR reviewed Aug 31, 2017
View reviewed changes

d2r mentioned this pull request Oct 16, 2018
STORM-3250: Closes Pull Requests unchanged in 2018 #2880
Merged

asfgit closed this in #2880 Oct 22, 2018

Uh oh!

Conversation

liu-zhaokun commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srdo commented Sep 19, 2017

Uh oh!

liu-zhaokun commented Sep 21, 2017

Uh oh!

srdo commented Sep 21, 2017

Uh oh!

liu-zhaokun commented Sep 23, 2017

Uh oh!

srdo commented Sep 23, 2017

Uh oh!

liu-zhaokun commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants