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

Batch append #229

Merged

thefringeninja merged 3 commits into master from batch-append

Nov 5, 2021

Member

George-Payne commented Nov 2, 2021 •

edited

Loading

Add batch append
decide if should batch append or append based off of support and credentials
update tests
http2-assertion-failure should use classic append
write-after-end should test both append styles (also fixes: tests: write-after-end.tests.ts assertion fails #215)
reconnect tests should use classic append

George-Payne self-assigned this

timothycoleman requested review from oskardudycz and thefringeninja

November 2, 2021 13:52

George-Payne force-pushed the batch-append branch 2 times, most recently from c818891 to e0d29a0 Compare

November 2, 2021 17:50

oskardudycz reviewed

View reviewed changes

src/__test__/connection/reconnect.test.ts Show resolved Hide resolved

oskardudycz reviewed

View reviewed changes

src/__test__/extra/write-after-end.test.ts Show resolved Hide resolved

oskardudycz reviewed

View reviewed changes

src/__test__/streams/appendToStream-batch-append.test.ts Outdated Show resolved Hide resolved

oskardudycz reviewed

View reviewed changes

src/__test__/streams/appendToStream-batch-append.test.ts Outdated Show resolved Hide resolved

oskardudycz reviewed

View reviewed changes

src/streams/appendToStream/index.ts Show resolved Hide resolved

oskardudycz reviewed

View reviewed changes

src/streams/appendToStream/unpackError.ts Show resolved Hide resolved

oskardudycz reviewed

View reviewed changes

Contributor

oskardudycz left a comment

I'm not sure if selecting batch vs regular append based on credentials is not too magical (there is no RFC), but as it's the same as in .NET, then I could approve it when the test coverage is increased.
See comments above.

George-Payne mentioned this pull request

Bulk up credentials tests #230

Open

George-Payne force-pushed the resolve-flakey-tests branch from 6732bfc to fa146da Compare

November 3, 2021 14:47

George-Payne force-pushed the batch-append branch from e0d29a0 to 58ab557 Compare

November 3, 2021 14:48

George-Payne force-pushed the resolve-flakey-tests branch from fa146da to 18a1029 Compare

November 3, 2021 18:01

George-Payne force-pushed the batch-append branch from 58ab557 to a149a27 Compare

November 3, 2021 18:01

Base automatically changed from resolve-flakey-tests to master

November 3, 2021 18:12

George-Payne added 2 commits

November 4, 2021 12:18


           Batch Append

4e764c1

 - Add batch append
 - decide if should batch append or append based off of support and credentials


          Update tests

9ed082e

- http2-assertion-failure should use classic append
- write-after-end should test both append styles
- reconnect tests should use classic append

George-Payne force-pushed the batch-append branch from a149a27 to 2249503 Compare

November 4, 2021 13:10


          Explicitly test append errors

- update test clusters to take extra options

George-Payne marked this pull request as ready for review

November 4, 2021 13:12

oskardudycz approved these changes

View reviewed changes

thefringeninja approved these changes

View reviewed changes

thefringeninja merged commit f9de3ba into master

thefringeninja deleted the batch-append branch

November 5, 2021 09:14

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

Labels

None yet