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

Re-spin of PR #1491: BTL TCP async progress #1497

Closed
wants to merge 4 commits into from
Closed

Re-spin of PR #1491: BTL TCP async progress #1497

wants to merge 4 commits into from

Conversation

jsquyres
Copy link
Member

@bosilca What do you think of this PR as a replacement for #1491? I did two things:

  1. Squashed most of the commits down and make Thananon the author (since you assumedly want him to get some credit, since the PR updates the AUTHOR files).
  2. Cleaned up the whitespace from the changes.

Ok?

Thananon Patinyasakdikul and others added 4 commits March 25, 2016 17:12
@jsquyres
btl tcp: Add async progress and fix threaded operations
f7bebe4 
* Added MCA parameter to turn progress thread on/off
* Add a flag to check if we have BTL progress thread.
* Added macro for ob1 matching lock.
* Removed stray debug printf.
* Changed the macro name to MCA_BTL_TCP_SUPPORT_PROGRESS_THREAD.
@jsquyres
Update the AUTHORS file.
e654dcb
@jsquyres
btl tcp: whitespace cleanup
18dbbd4 
No code changes

Signed-off-by: Jeff Squyres <[email protected]>
@jsquyres
pml ob1: whitespace cleanup
e836d7b 
No code changes

Signed-off-by: Jeff Squyres <[email protected]>
@jsquyres
Copy link
Member Author

@bosilca Does this code do what was suggested in #1406 (comment)?

@bosilca
Copy link
Member

bosilca commented Mar 26, 2016

I don't understand on what grounds are you altering someone else PR ?

@jsquyres
Copy link
Member Author

I didn't alter your PR. I have suggestions for your PR, so I filed this new one. Here's why:

  • Your initial PR had a few dozen commits.
  • You later closed that commit and squashed the commits down and filed PR BTL TCP async progress #1491.
  • While reading that PR, I realized that you still had commits that added a bunch of code and then removed that same code. I also saw a bunch of whitespace problems.
  • Rather than suggest to you to make those changes, I figured I could save you some time and propose a new PR with exactly what I was suggesting: further squashing (to avoid the net-zero-change commits), and fix the whitespace problems.

It's just a suggestion, George.

Can you answer whether this PR (or #1491) does what was suggested in #1406 (comment)?

@jsquyres jsquyres changed the title Re-spin of PR #1491 Re-spin of PR #1491: BTL TCP async progress Mar 26, 2016
@bosilca
Copy link
Member

bosilca commented Mar 28, 2016

Thanks Jeff, but #1491 has been reduced to 4 commits for quite a few days now. I added there the space cleanup so I am not sure we need ticket this anymore.

@bosilca bosilca closed this Mar 28, 2016
@jsquyres
Copy link
Member Author

Excellent; thanks @bosilca

@jsquyres jsquyres deleted the ICLDisco-progress_thread branch March 28, 2016 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jsquyres @bosilca