Skip to content

[STORM-2713] when the connection to the first zkserver is timeout,storm-kafka's kafkaspout will throw a exception #2297

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.

Sign up for GitHub

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

Closed
liu-zhaokun wants to merge 1 commit into from
Closed

[STORM-2713] when the connection to the first zkserver is timeout,storm-kafka's kafkaspout will throw a exception #2297

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion external/storm-kafka/src/jvm/org/apache/storm/kafka/DynamicBrokersReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ private int getNumPartitions(String topic) {
List<String> children = _curator.getChildren().forPath(topicBrokersPath);
return children.size();
} catch (Exception e) {
throw new RuntimeException(e);
LOG.error(e.getMessage(), e);

@HeartSaVioR HeartSaVioR Aug 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HeartSaVioR HeartSaVioR Aug 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@liu-zhaokun liu-zhaokun Aug 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@liu-zhaokun liu-zhaokun Aug 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@HeartSaVioR HeartSaVioR Aug 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HeartSaVioR HeartSaVioR Aug 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@liu-zhaokun liu-zhaokun Aug 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@liu-zhaokun liu-zhaokun Aug 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
}

Expand Down