Prepare client.c for use in the client library #458

cole-miller · 2023-01-17T22:39:05Z

src/client.{c,h} -> src/client/protocol.{c,h} in preparation for more client source files
Implement remaining request/response types (that are actually used by go-dqlite)
Use up-to-date protocol/schema versions for all requests
Improved error handling
Timeouts for reading from the underlying fd

This will be easiest to review commit-by-commit.

codecov · 2023-01-17T22:43:49Z

Codecov Report

Merging #458 (f265305) into master (86ac733) will decrease coverage by 2.24%.
The diff coverage is 48.59%.

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
- Coverage   74.32%   72.08%   -2.24%     
==========================================
  Files          32       33       +1     
  Lines        5382     5878     +496     
  Branches     1682     1871     +189     
==========================================
+ Hits         4000     4237     +237     
- Misses        823     1004     +181     
- Partials      559      637      +78

Impacted Files	Coverage Δ
src/transport.c	`70.77% <ø> (+5.84%)`	⬆️
src/client/protocol.c	`48.42% <48.42%> (ø)`
src/gateway.c	`60.91% <66.66%> (+0.50%)`	⬆️
src/request.c	`100.00% <100.00%> (ø)`
src/conn.c	`71.92% <0.00%> (-1.45%)`	⬇️
src/leader.c	`69.31% <0.00%> (-0.77%)`	⬇️
src/id.c	`100.00% <0.00%> (ø)`
src/fsm.c	`75.04% <0.00%> (+0.04%)`	⬆️
src/vfs.c	`84.12% <0.00%> (+0.14%)`	⬆️
src/server.c	`68.60% <0.00%> (+0.41%)`	⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Cole Miller <cole.miller@canonical.com>

This information doesn't seem useful to me. Signed-off-by: Cole Miller <cole.miller@canonical.com>

We probably want to save the generic name for a higher-level object. Signed-off-by: Cole Miller <cole.miller@canonical.com>

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller · 2023-01-23T17:39:56Z

@freeekanayaka, could you elucidate what DQLITE_REQUEST_CONNECT is for? It seems that it's not used at all by go-dqlite, so I didn't implement it here. There's still a codepath in dqlite to handle such requests -- perhaps it could be removed?

test/integration/test_cluster.c

src/client/protocol.c

MathieuBordere · 2023-01-24T11:10:31Z

src/client/protocol.c

+ * On error, -1 is returned. Otherwise the return value is the count
+ * of bytes read. This may be less than n if either EOF happened or
+ * polling timed out. */
+static ssize_t doRead(int fd, void *buf, size_t n, int timeout_millis)


Maybe we can pass the client struct here instead and do some accounting on the timeout and save e.g. timeout_left in the client struct after measuring the time spent in poll?

Yes, this makes sense to me, I'll try to implement it. (The alternative for setting a budget on the time spent in the loop as a whole would be arranging to get SIGALRM when a timer expires and setting up a handler, but that sounds terrible.)

MathieuBordere · 2023-01-24T12:00:01Z

Did a first round of reading through the changes, think it already looks good.

freeekanayaka · 2023-01-25T09:05:36Z

@freeekanayaka, could you elucidate what DQLITE_REQUEST_CONNECT is for? It seems that it's not used at all by go-dqlite, so I didn't implement it here. There's still a codepath in dqlite to handle such requests -- perhaps it could be removed?

It's basically used as glue between dqlite and raft. If you use dqlite without a custom transport, then when raft needs to establish a connection to another raft node it will use the default dqlite transport defined in transport.c which basically connects to the desired dqlite engine running the raft code and sends the DQLITE_REQUEST_CONNECT command to tell the dqlite engine to handover the connection to the raft code.

It's implicitly used by go-dqlite as well via dqlite_node_set_connect_func.

I hope my memory doesn't fail me and that everything is correct and clear :) Please let me know if something doesn't match.

cole-miller · 2023-01-25T14:38:40Z

@freeekanayaka Got it, thanks! I'll add this to the docs.

What's the motivation for proxying raft connections through dqlite, instead of having them flow independently? I guess it's convenient to have just one address that both dqlite and raft are listening on.

freeekanayaka · 2023-01-27T19:14:43Z

@freeekanayaka Got it, thanks! I'll add this to the docs.

What's the motivation for proxying raft connections through dqlite, instead of having them flow independently? I guess it's convenient to have just one address that both dqlite and raft are listening on.

Sorry, I didn't see your follow up before. Yes, it's to have a single endpoint.

cole-miller · 2023-01-30T14:52:54Z

I've pushed another (rather large, sorry) commit that implements a different strategy for timing out reads/writes. It also includes a rewrite of clientRecvRows to do proper cleanup if an error is encountered.

MathieuBordere

Mostly nits :-)

src/client/protocol.h

MathieuBordere · 2023-01-31T09:31:42Z

src/client/protocol.c

+{
+	switch (val.type) {
+		case SQLITE_TEXT:
+			free((char *)val.text);


Funny that you need to cast this :-)

Yes, I guess not being allow to free const T* is some kind of safeguard against calling it on static data.

src/client/protocol.c

.gitignore

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller changed the title ~~Start setting up the C client~~ Prepare client.c for use in the client library Jan 18, 2023

cole-miller force-pushed the cclient branch from e58cc45 to 6da6275 Compare January 18, 2023 20:55

cole-miller added 7 commits January 19, 2023 14:55

client: Move to a subdirectory

8fba7a5

Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Don't trace fd

3db48c9

This information doesn't seem useful to me. Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Rename struct client

cbebf24

We probably want to save the generic name for a higher-level object. Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Update include guard

ee0d499

Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Handshake version 1

5523ba8

Signed-off-by: Cole Miller <cole.miller@canonical.com>

request: Rename *_ASSIGN to *_PROMOTE_OR_ASSIGN

8421bd6

Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Properly implement clientSendAssign

8d58778

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller force-pushed the cclient branch 3 times, most recently from 6cd4a4d to 291f6df Compare January 19, 2023 20:05

cole-miller added 2 commits January 19, 2023 15:10

client/protocol: Implement statement parameters

0238acb

Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Don't use the sqlite3 allocator

2088754

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller force-pushed the cclient branch 3 times, most recently from f7516ed to 6ffe509 Compare January 20, 2023 18:15

cole-miller added 3 commits January 20, 2023 16:06

client/protocol: Expose cursor from RESPONSE macro

13c9ce6

Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Handle failure message from server

27baf5e

Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Handle short reads

70d7e81

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller force-pushed the cclient branch from 6ffe509 to 75570b7 Compare January 20, 2023 21:06

client/protocol: Record DB filename

90abd9c

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller force-pushed the cclient branch 2 times, most recently from afdb1b6 to 036c96d Compare January 20, 2023 21:22

cole-miller added 4 commits January 23, 2023 09:21

client/protocol: Implement remaining request types

3ff8317

Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Implement remaining response types

1206411

Signed-off-by: Cole Miller <cole.miller@canonical.com>

client/protocol: Timeout for read operations

fa1a635

Signed-off-by: Cole Miller <cole.miller@canonical.com>

client: Add some more tests

e1819a1

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller force-pushed the cclient branch from 036c96d to e1819a1 Compare January 23, 2023 14:22

cole-miller requested a review from MathieuBordere January 23, 2023 14:22

cole-miller mentioned this pull request Jan 23, 2023

Updates and corrections to wire protocol docs canonical/dqlite-docs#7

Merged

MathieuBordere reviewed Jan 24, 2023

View reviewed changes

cole-miller force-pushed the cclient branch 2 times, most recently from 591af7e to 8a2e2e6 Compare January 26, 2023 23:30

cole-miller force-pushed the cclient branch from 8a2e2e6 to 41867d3 Compare January 30, 2023 14:41

cole-miller marked this pull request as ready for review January 30, 2023 14:53

cole-miller requested a review from MathieuBordere January 30, 2023 14:53

MathieuBordere reviewed Jan 31, 2023

View reviewed changes

cole-miller force-pushed the cclient branch from 41867d3 to 9282fb0 Compare January 31, 2023 16:33

cole-miller requested a review from MathieuBordere January 31, 2023 16:34

client/protocol: Better timeouts, other fixes

cf94843

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller force-pushed the cclient branch from 9282fb0 to cf94843 Compare January 31, 2023 16:41

request: Add a note about PROMOTE_OR_ASSIGN

f265305

Signed-off-by: Cole Miller <cole.miller@canonical.com>

MathieuBordere merged commit d34e461 into canonical:master Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare client.c for use in the client library #458

Prepare client.c for use in the client library #458

cole-miller commented Jan 17, 2023 •

edited

Loading

codecov bot commented Jan 17, 2023 •

edited

Loading

cole-miller commented Jan 23, 2023

MathieuBordere Jan 24, 2023

cole-miller Jan 24, 2023 •

edited

Loading

MathieuBordere commented Jan 24, 2023

freeekanayaka commented Jan 25, 2023

cole-miller commented Jan 25, 2023 •

edited

Loading

freeekanayaka commented Jan 27, 2023

cole-miller commented Jan 30, 2023

MathieuBordere left a comment

MathieuBordere Jan 31, 2023

cole-miller Jan 31, 2023

Prepare client.c for use in the client library #458

Prepare client.c for use in the client library #458

Conversation

cole-miller commented Jan 17, 2023 • edited Loading

codecov bot commented Jan 17, 2023 • edited Loading

Codecov Report

cole-miller commented Jan 23, 2023

MathieuBordere Jan 24, 2023

Choose a reason for hiding this comment

cole-miller Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

MathieuBordere commented Jan 24, 2023

freeekanayaka commented Jan 25, 2023

cole-miller commented Jan 25, 2023 • edited Loading

freeekanayaka commented Jan 27, 2023

cole-miller commented Jan 30, 2023

MathieuBordere left a comment

Choose a reason for hiding this comment

MathieuBordere Jan 31, 2023

Choose a reason for hiding this comment

cole-miller Jan 31, 2023

Choose a reason for hiding this comment

cole-miller commented Jan 17, 2023 •

edited

Loading

codecov bot commented Jan 17, 2023 •

edited

Loading

cole-miller Jan 24, 2023 •

edited

Loading

cole-miller commented Jan 25, 2023 •

edited

Loading