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

Add DataStreamBidirectional API #209

Merged
merged 32 commits into from
Mar 5, 2024
Merged

Add DataStreamBidirectional API #209

merged 32 commits into from
Mar 5, 2024

Conversation

corbin-phipps
Copy link
Contributor

Type

  • Bug fix
  • Feature addition
  • Feature update
  • Documentation
  • Build Infrastructure

Side Effects

  • Breaking change
  • Non-functional change

Goals

This PR adds the DataStreamBidirectional API and unit tests. Also fixes a race condition bug and adds handling of sequencing mismatches.

Technical Details

  • Added DataStreamBidirectional API to NetRemoteDataStreamingService.proto.
  • Added server-side DataStreamReaderWriter implementation of the ServerBidiReactor gRPC reactor.
  • Added client-side DataStreamReaderWriter implementation of the ClientBidiReactor gRPC reactor.
  • Added two unit tests - one for fixed-type client streaming and one for continuous-type client streaming.
  • Added handling for mismatched sequencing.
  • Fixed a race condition bug affecting DataStreamUpload.

Test Results

All tests pass.

Reviewer Focus

None.

Future Work

None.

Checklist

  • Build target all compiles cleanly.
  • clang-format and clang-tidy deltas produced no new output.
  • Newly added functions include doxygen-style comment block.

corbin-phipps and others added 28 commits March 1, 2024 19:10
@corbin-phipps
Add bidi API to proto
5081a44
@corbin-phipps
Add placeholder for bidi API impl
da2bfda
@corbin-phipps
Add placeholder bidi reactor
a6b88a3
@corbin-phipps
Add initial reactor impl without synchronization of m_status
634ef15
@corbin-phipps
Add initial client bidi reactor
4a801c7
@corbin-phipps
Add basic unit test
7c5fea6
@corbin-phipps
Fix race condition
2966971
@corbin-phipps
Add numberOfDataBlocksToStream to Await() function
e412d85
@corbin-phipps
clang-format and clang-tidy
95e7c88
@corbin-phipps
clang-tidy fix
3fda245
@corbin-phipps
clang-format and clang-tidy changes
1cbd324
@corbin-phipps
Merge pull request #202 from microsoft/user/corbinphipps/add-bidi-api
ba994ad 
Add DataStreamBidirectional API
@corbin-phipps
Merge branch 'develop' into feature/dataStreaming
dfdd261
@corbin-phipps
Track lost data block sequence numbers in test client code
d4a9dc2
@corbin-phipps
Apply change to bidi API as well
cda9bef
@corbin-phipps
Replace vector output param with std::span
c637e53
@corbin-phipps
clang-tidy
e519ed8
@corbin-phipps
Directly initialize span without iterating
5d59d62
@corbin-phipps
Merge pull request #205 from microsoft/user/corbinphipps/mismatch-seq…
c454a9b 
…uencing

Check when mismatched sequencing occurs
@corbin-phipps
Move decrement line above StartWrite call
9c09322
@corbin-phipps
Apply change to bidi API too
1e10655
@corbin-phipps
Merge pull request #207 from microsoft/user/corbinphipps/fix-upload-bug
79e1f27 
Fix intermittent failure in DataStreamUpload
@corbin-phipps
Add Cancel() to DataStreamReaderWriter client reactor
e094ce5
@corbin-phipps
Update client reactor to take in DataStreamProperties
95cad9d
@corbin-phipps
Add StopWrites() to stop writing data from client to server
834066d
@corbin-phipps
Don't call Finish in OnCancel
0756366
@corbin-phipps
clang-tidy
829183b
@corbin-phipps
Merge pull request #208 from microsoft/user/corbinphipps/add-cancelat…
8395123 
…ion-test

Continuous client-side streaming using DataStreamBidirectional API
@corbin-phipps corbin-phipps requested a review from a team as a code owner March 4, 2024 23:02
@abeltrano
Copy link
Contributor

abeltrano commented Mar 5, 2024 •
edited
Loading

  • Fixed a race condition bug affecting DataStreamUpload.

Was this the fix done in #207? If so, can we add a test to ensure it's fixed and stays fixed?

abeltrano
abeltrano requested changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@abeltrano abeltrano left a comment

Choose a reason for hiding this comment

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

I added some comments/questions in #205 to address before we complete this.

@corbin-phipps
Copy link
Contributor Author

  • Fixed a race condition bug affecting DataStreamUpload.

Was this the fix done in #207? If so, can we add a test to ensure it's fixed and stays fixed?

Yes, the fix was in #207. The bug was found by the existing unit tests. I'm not sure how to specifically test this bug fix since it would likely do the same thing as the existing tests.

@corbin-phipps
Copy link
Contributor Author

I added some comments/questions in #205 to address before we complete this.

Thanks, will add the changes here

@abeltrano
Copy link
Contributor

I added some comments/questions in #205 to address before we complete this.

Thanks, will add the changes here

Please make sure the change is merged back to the feature branch. Can probably cherry-pick it.

@abeltrano
Copy link
Contributor

  • Fixed a race condition bug affecting DataStreamUpload.

Was this the fix done in #207? If so, can we add a test to ensure it's fixed and stays fixed?

Yes, the fix was in #207. The bug was found by the existing unit tests. I'm not sure how to specifically test this bug fix since it would likely do the same thing as the existing tests.

If the existing unit tests would find it consistently, then we're good.

@abeltrano abeltrano merged commit 0fae759 into develop Mar 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abeltrano abeltrano abeltrano approved these changes

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
@corbin-phipps @abeltrano