Skip to content
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.

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

Contribute Custom Erlang Network Protocol to CouchDB #3643

Merged
merged 1 commit into from Jul 28, 2021
Merged

Contribute Custom Erlang Network Protocol to CouchDB #3643

merged 1 commit into from Jul 28, 2021

Conversation

jiahuili430
Copy link
Contributor

@jiahuili430 jiahuili430 commented Jun 25, 2021

Overview

Enable Custom Erlang Network Protocol for CouchDB Classic.

  • Add the couch_dist.erl app with tests.
  • Update the vm.args config lines with documentation.
  • Updateremsh.tls script, configure for development, and release script remsh

Testing recommendations

make eunit apps=couch_dist suites=couch_dist_tests

Related Issues or Pull Requests

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

rel/overlay/etc/vm.args Outdated Show resolved Hide resolved
@wohali
Copy link
Member

wohali commented Jun 26, 2021

@jiahuili430 Did you mean to close this? It seems promising!

@jiahuili430
Copy link
Contributor Author

I will reopen this PR, and try to finish it next week.

@jiahuili430 jiahuili430 reopened this Jun 28, 2021
@nickva
Copy link
Contributor

nickva commented Jun 29, 2021

Do we recall the reason we couldn't use the Erlang provided tls_dist as described here: http://www1.erlang.org/doc/apps/ssl/ssl_distribution.html and use a custom module?

Also currently it seems this module won't be used anywhere it's not referenced from any place in the code (even in comments)

@nickva
Copy link
Contributor

nickva commented Jul 1, 2021

It still seems we are just proxy-ing the callbacks directly to the Erlang/OTP provided inet_tls module? So our module is not doing anything differently than the legacy behavior as described in https://erlang.org/doc/apps/ssl/ssl_distribution.html. There is even a newer way of specifying just a single conf file, which seems nice -proto_dist inet_tls -ssl_dist_optfile "/path/to/tls.conf" but perhaps we can just refer to those docs in the vm.arg or docs files.

rel/overlay/etc/vm.args Outdated Show resolved Hide resolved
@jiahuili430
Copy link
Contributor Author

jiahuili430 commented Jul 2, 2021

@nickva

It still seems we are just proxy-ing the callbacks directly to the Erlang/OTP provided inet_tls module? So our module is not doing anything differently than the legacy behavior as described in https://erlang.org/doc/apps/ssl/ssl_distribution.html. There is even a newer way of specifying just a single conf file, which seems nice -proto_dist inet_tls -ssl_dist_optfile "/path/to/tls.conf" but perhaps we can just refer to those docs in the vm.arg or docs files.

I haven't finished yet, still working on it. Just add TCP.
Thanks for the review

@nickva
Copy link
Contributor

nickva commented Jul 2, 2021

@jiahuili430

I haven't finished yet, still working on it. Just add TCP.

ah, that makes sense

src/couch_dist/LICENSE Outdated Show resolved Hide resolved
dev/remsh Outdated Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
rel/overlay/etc/vm.args Outdated Show resolved Hide resolved
dev/remsh.tls Outdated Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
rel/overlay/etc/vm.args Outdated Show resolved Hide resolved
rel/overlay/etc/vm.args Outdated Show resolved Hide resolved
rel/overlay/bin/remsh Outdated Show resolved Hide resolved
rel/overlay/bin/remsh Outdated Show resolved Hide resolved
rel/overlay/etc/vm.args Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile.win Outdated Show resolved Hide resolved
@jiahuili430 jiahuili430 changed the title Contribute Custom Erlang network protocol to Apache Contribute Custom Erlang Network Protocol to CouchDB Jul 26, 2021
@jiahuili430
Copy link
Contributor Author

Thanks to everyone who helped me complete this PR, especially @nickva, @iilyak, and @rnewson.

@nickva found the bug in listen/2, we cannot use node() to return node info. Because when setting up CouchDB, node() cannot return the name of the node because it is not connected to CouchDB yet.
@iilyak, he helped the software design part to make the application more scalable and extensible.
Thank you, @rnewson. Without your code, we cannot figure out how to enable custom erlang network protocol for CouchDB.

@jiahuili430 jiahuili430 marked this pull request as ready for review July 27, 2021 15:04
Copy link
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

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

+1 Nice work!

@nickva
Copy link
Contributor

nickva commented Jul 27, 2021

We'd want documentation PR as well probably. It could probably re-iterate some of the comments in the vm.args file

@jiahuili430
Copy link
Contributor Author

We'd want documentation PR as well probably. It could probably re-iterate some of the comments in the vm.args file

Add documentation: apache/couchdb-documentation#673

dev/remsh.tls Outdated Show resolved Hide resolved
src/couch_dist/src/couch_dist.erl Outdated Show resolved Hide resolved
@nickva
Copy link
Contributor

nickva commented Jul 28, 2021

Well done, @jiahuili430. Merging to 3.x

@nickva nickva merged commit 6f7b779 into apache:3.x Jul 28, 2021
@jiahuili430 jiahuili430 deleted the 699-custom-erlang-network-protocol branch July 28, 2021 21:49
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.

None yet

6 participants