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.

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

[tcp-close] New whocloses semantics #630

Merged
merged 2 commits into from
Apr 11, 2023
Merged

[tcp-close] New whocloses semantics #630

merged 2 commits into from
Apr 11, 2023

Conversation

anandbonde
Copy link
Contributor

@anandbonde anandbonde commented Apr 10, 2023 •
edited

Closes #526

@anandbonde anandbonde self-assigned this Apr 10, 2023
@ppenna ppenna changed the title [tcp-close] Enhancement: New whocloses semantics [tcp-close] New whocloses semantics Apr 10, 2023
@ppenna ppenna added the enhancement Enhancement Request on an Existing Feature label Apr 10, 2023
@ppenna ppenna self-requested a review April 10, 2023 18:40
@ppenna
Copy link
Contributor

ppenna commented Apr 10, 2023

@anandbonde could you change the description of this issue so that it links issues correctly? https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

ppenna
ppenna reviewed Apr 10, 2023
View reviewed changes
Copy link
Contributor

@ppenna ppenna left a comment

Choose a reason for hiding this comment

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

Tests are not passing. Why? Could you file an issue to keep track of this bug? If you disable this test for some LibOSes, could you file an issue to keep track which LibOSes fall in this condition?

@anandbonde anandbonde force-pushed the enhancement-new-semantics-for-tcp-close branch from 61455b7 to 5faee03 Compare April 10, 2023 19:15
@anandbonde
Copy link
Contributor Author

anandbonde commented Apr 10, 2023 •
edited

@anandbonde could you change the description of this issue so that it links issues correctly? https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

Done

@anandbonde anandbonde force-pushed the enhancement-new-semantics-for-tcp-close branch from 5faee03 to 8711b5a Compare April 10, 2023 19:31
@anandbonde
Copy link
Contributor Author

anandbonde commented Apr 10, 2023 •
edited

Tests are not passing. Why? Could you file an issue to keep track of this bug? If you disable this test for some LibOSes, could you file an issue to keep track which LibOSes fall in this condition?

I just checked the logs, the tests are failing because I have added a new argument to the tcp-close utility, but the ci scripts are not updated to use it. I think it would be good to change the ci scripts to be updated in this PR itself so that we don't have to revisit this area. Do you have any preference?

@ppenna
Copy link
Contributor

ppenna commented Apr 10, 2023

Please, update the CI scripts accordingly. I am also confident that this will not work for all LibOSes (it should expose bugs). For those LibOSes which the CI does not pass, disable them and file an issue to keep track of.

iyzhang
iyzhang approved these changes Apr 10, 2023
View reviewed changes
Copy link
Contributor

@iyzhang iyzhang left a comment

Choose a reason for hiding this comment

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

LGTM

examples/tcp-close/client.rs Show resolved Hide resolved
@anandbonde anandbonde force-pushed the enhancement-new-semantics-for-tcp-close branch from 8711b5a to 6ac6b2f Compare April 11, 2023 06:01
ppenna
ppenna requested changes Apr 11, 2023
View reviewed changes
tools/demikernel_ci.py Outdated Show resolved Hide resolved
@anandbonde anandbonde force-pushed the enhancement-new-semantics-for-tcp-close branch from 6ac6b2f to e5bfc8c Compare April 11, 2023 20:48
ppenna
ppenna approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@ppenna ppenna left a comment

Choose a reason for hiding this comment

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

LGTM once the issue on remove() is fixed.

@anandbonde anandbonde merged commit 46c92ad into dev Apr 11, 2023
10 checks passed
@anandbonde anandbonde deleted the enhancement-new-semantics-for-tcp-close branch April 11, 2023 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppenna ppenna ppenna approved these changes

@iyzhang iyzhang iyzhang approved these changes

Assignees

@anandbonde anandbonde

Labels
enhancement Enhancement Request on an Existing Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[test] Integration Tests for TCP close()
3 participants
@anandbonde @ppenna @iyzhang