Skip to content

HDDS-5237. Add SSL support to the Ozone streaming API#2315

Merged
elek merged 7 commits intoapache:masterfrom
elek:streaming-ssl
Jun 21, 2021
Merged

HDDS-5237. Add SSL support to the Ozone streaming API#2315
elek merged 7 commits intoapache:masterfrom
elek:streaming-ssl

Conversation

@elek
Copy link
Member

@elek elek commented Jun 8, 2021

What changes were proposed in this pull request?

HDDS-5142 will introduce a new streaming API for closed container replication / snapshot download and other data movement.

For server2server communication we need to support mTLS. We should configure pure mTLS on the netty server

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-5237

How was this patch tested?

New unit test is added to test SSL option with self-signed certificate. During the tests I also found some intermittent issues in the DirstreamerClient. I added more unit tests there with minor changes.

@adoroszlai adoroszlai changed the title HDDS-5303. Add SSL support to the Ozone streming API HDDS-5237. Add SSL support to the Ozone streming API Jun 8, 2021
@adoroszlai adoroszlai changed the title HDDS-5237. Add SSL support to the Ozone streming API HDDS-5237. Add SSL support to the Ozone streaming API Jun 8, 2021
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @elek for the improvement, LGTM. Nice tests. Only some minor code items noted.

Comment on lines +96 to +97
} catch (InterruptedException ex) {
throw new RuntimeException(ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep throws InterruptedException instead? I think it's better for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

InterrputedException is a checked exception what I tried to avoid, especially as we don't see any added value in differentiating between this error or any other runtime error during the startup.

I would be happy to replace it with any more specific runtime exception -- if you have any suggestion -- but not sure how would it be more clear with keeping checked execption,

I tried how would it look like with keeping the checked InterruptedException here, but it doesn't look more clear to me, it requires same conversation (but later) plus maintaining additional checked execption in the method signature.

Looks to be more clear for me simple use runtime exception instead of checked one (BTW, the current practice to convert everything to IOException is also more closed to use RuntimeExceptions everywhere and do the differentating only if it's required.

Uploaded the experiment to here: elek@767b5fb, I can add it to this PR, if this is your strong preference...

Copy link
Contributor

Choose a reason for hiding this comment

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

So if I understand correctly, the current patch itself does not need to handle InterruptedException, but a future change (replacing the current replication server implementation with the streaming one) will. So this is in anticipation of the next change.

If that's correct, I'm OK with converting it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed a new commit (0448a98) with another approach based on our offline discussion.

I created a StreamingException (runtime) which can be used instead of the generic RuntimeException but still unchecked.

Furthermore, I also updated existing streaming API to avoid using raw new RuntimeException everywhere in the streaming package.

Please let me know what do you think....

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating the patch. I think it's much cleaner.

@elek
Copy link
Member Author

elek commented Jun 21, 2021

Thanks for the review @adoroszlai. I am merging it now...

@elek elek merged commit c0eb347 into apache:master Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants