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

fix(client): Fix client using localhost for chunkservers #235

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

uristdwarf
Copy link
Collaborator

When master gives information regarding chunks and chunkservers, it may give the localhost ip addresses of chunkserver to client, causing the client to search for the chunks on their localhost. This commit fixes that by interpreting localhost from master to mean the master IP address.

Copy link
Collaborator

@rolysr rolysr left a comment

Choose a reason for hiding this comment

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

LGTM. But consider my minor suggestions for code readabilty. On the other hand, did you considered if it is necessary to add a test for checking the fix of the issue that was happening before? Just for checking there is not edge case failing for this.

antuan96314
antuan96314 previously approved these changes Nov 6, 2024
Copy link
Contributor

@ralcolea ralcolea left a comment

Choose a reason for hiding this comment

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

LGTM. Please, consider adding an automated test where at least one chunkserver use localhost as IP to validate the change.

@uristdwarf
Copy link
Collaborator Author

LGTM. Please, consider adding an automated test where at least one chunkserver use localhost as IP to validate the change.

I tried adding one, but it isn't simple (with our current testing framework). This problem occurs because of a client on a different host.

You have chunkserver/master on host A, and client on host B. Master returns the chunkserver ip address relative to it's own address on host B. However with the testing framework everything is tested on the single host, where this bug doesn't happen. If I researched this more I'm sure I could find a way to emulate this behavior, but I don't think it's worth the time or effort

@uristdwarf
Copy link
Collaborator Author

uristdwarf commented Jan 8, 2025

If I researched this more I'm sure I could find a way to emulate this behavior, but I don't think it's worth the time or effort

At least not for this PR, which is my personal contribution I made because I found and fixed it in my spare time.

uristdwarf and others added 5 commits February 26, 2025 16:09
When master gives information regarding chunks and chunkservers, it may
give the localhost ip addresses of chunkserver to client, causing the
client to search for the chunks on their localhost. This commit fixes
that by interpreting localhost from master to mean the master IP
address.

Signed-off-by: Urmas Rist <[email protected]>
Signed-off-by: Urmas Rist <[email protected]>

Co-authored-by: Antuan <[email protected]>
Signed-off-by: Urmas Rist <[email protected]>

Co-authored-by: aNeutrino <[email protected]>
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.

5 participants