[FLINK-4053] Return value from Connection should be checked against null #2128
Conversation
| * @throws IOException if failed to open RabbitMQ channel | ||
| * @throws RuntimeException if connection object returned null channel | ||
| */ | ||
| public static Channel createChannel(Connection connection) throws IOException { |
There was a problem hiding this comment.
a separate class and static method is a bit overkill for a null check.
|
I wanted to avoid this bit of duplication, but I have no strong preferences for that. Will remove this static method. |
| } | ||
|
|
||
| @Test | ||
| public void throwExceptionIfChannenIsNull() throws Exception { |
|
@zentol I've fixed my PR according to your comments. |
| @@ -76,6 +76,9 @@ public void open(Configuration config) throws Exception { | |||
| try { | |||
| connection = factory.newConnection(); | |||
| channel = connection.createChannel(); | |||
| if (channel == null) { | |||
There was a problem hiding this comment.
Do you know under which circumstances this can occur and/or what a user can do to resolve it?
There was a problem hiding this comment.
As far as I understand in RabbitMQ every real TCP connection has a number of virtual connections implemented on top of it. If we request a channel and it's not available "createChannel" returns null.
I've updated the exception message to reflect that.
I don't think this generally will be an issue, since we only request one channel for a connection, but it would be beneficial to throw an exception with an appropriate message instead of a NullPointerException in case if anything goes wrong.
|
I tihnk this looks good. One remark for future contributions though: I think some of your tests are too specific. Effectively you are not testing functionality but putting a lock on every single line of the implementation. We will now get a test failure (which should mean that the functionality is broken) if we change the exception message (or even handle it completely), use a different |
|
@zentol I see your point. Do you suggest to remove these tests? |
|
@zentol Should I update this PR somehow? |
|
Thank your for reminding me, i forgot about this PR a bit :( The PR is fine imo, +1 to merge. |
|
Hi all. If this is fine and useful, could someone please merge this? |
|
merging |
|
Thank you @zentol ! |
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.
mvn clean verifyhas been executed successfully locally or a Travis build has passed