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

Jump to bottom

Convert 0.0.0.0 to local ip addresses when matching the cluster node #1392

Merged

PragmaTwice merged 7 commits into apache:unstable from zevin02:fix-ipaddr

Apr 20, 2023

Contributor

zevin02 commented Apr 17, 2023

fixes: #1381

Add a function to get all local IP addresses

torwig reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

torwig reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

Contributor

torwig commented Apr 17, 2023 •

edited

Loading

@zevin02 It's very handy to run ./x.py format before committing the changes - it will format the C++ source code automatically so the CI's linter will be happy.

Member

PragmaTwice commented Apr 17, 2023 •

edited

Loading

Hi @zevin02, thanks for your contribution!

We setup lots of code checks in our CI, which requires you to pass for better code quality.

For example, from a glance to your code, maybe these checks will fail:

local variables must be lower_case with underscore,
NULL must be replaced with nullptr

You can check the CI log for more details and for how to modify your code to pass the CI.

It is recommended to try some commands to solve these problems, but maybe not all problems:

./x.py format
./x.py check tidy --fix

Another thing is that, it would be better to have some unit tests for this function before it is merged : )

git-hulk requested a review from PragmaTwice

April 18, 2023 02:02

enjoy-binbin reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved


          feat: implement local IP address retrieval function

bfceb58

fixes: apache#1381

- Add a function to get all local IP addresses

Signed-off-by: Zewen Xu <zevin9427@gmail.com>

zevin02 force-pushed the fix-ipaddr branch from fe0a460 to bfceb58 Compare

April 18, 2023 12:08

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved


          refactor: refactor network functions into common library

9cb024a

- Move the `GetLocalIpAddresses` function to `common/io_util.cc`
- Add `common/io_util.h` header to `cluster/cluster.h`
- Use `util::GetLocalIpAddresses()` instead of the previously defined function in `cluster.cc`

Signed-off-by: Zewen Xu <zevin9427@gmail.com>

Contributor Author

zevin02 commented Apr 19, 2023

@PragmaTwice I think I solved all the problems, can you give me a review

Member

PragmaTwice commented Apr 19, 2023 •

edited

Loading

@PragmaTwice I think I solved all the problems, can you give me a review

Sure! But as I mentioned in #1392 (comment), you need first to make the CI pass, which currently failed.

./x.py format

zevin02 and others added 2 commits

April 19, 2023 18:46


          Format code

a6212e7

Signed-off-by: Zewen Xu <zevin9427@gmail.com>


          Merge branch 'unstable' into fix-ipaddr

f088f68

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

src/common/io_util.h Outdated Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.h Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed

View reviewed changes

Member

PragmaTwice left a comment

The rest looks good to me. cc @git-hulk

git-hulk added 2 commits

April 19, 2023 22:36


          Minor refactor the match listening ip

4390d5e


          Fix include headers

Member

git-hulk commented Apr 19, 2023

The rest looks good to me. cc @git-hulk

@PragmaTwice I do a minor refactor, can help to take a look again.

git-hulk requested review from torwig and PragmaTwice

April 19, 2023 15:11

git-hulk requested a review from enjoy-binbin

April 19, 2023 15:11

git-hulk changed the title ~~feat: implement local IP address retrieval function~~ Convert 0.0.0.0 to local ip addresses when matching the cluster node

torwig reviewed

View reviewed changes

Contributor

torwig left a comment

Generally, LGTM, but I have a few comments.

src/common/io_util.cc Outdated Show resolved Hide resolved

src/common/io_util.cc Outdated Show resolved Hide resolved

src/common/io_util.cc Outdated Show resolved Hide resolved


          refactor: refactor network address functions

e8bb1bf

- Check for IPv6 addresses in `MatchListeningIP`
- Remove unnecessary comment about loopback addresses in `GetLocalIPAddresses`

Signed-off-by: Zewen Xu <zevin9427@gmail.com>

torwig approved these changes

View reviewed changes

Contributor

torwig left a comment

LGTM

git-hulk approved these changes

View reviewed changes

PragmaTwice approved these changes

View reviewed changes

Member

PragmaTwice commented Apr 20, 2023

Thanks for your contribution! Merging...

PragmaTwice merged commit 13b8287 into apache:unstable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet