Skip to content

Conversation

@mtopolnik
Copy link
Contributor

@mtopolnik mtopolnik commented Mar 22, 2024

Closes #61. Closes #60.

@mtopolnik mtopolnik requested review from amunra and jerrinot March 22, 2024 16:51
Copy link
Collaborator

@amunra amunra left a comment

Choose a reason for hiding this comment

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

Substantial improvements.
Some minor tweaks recommended.
Note that the C, C++ and FFI doc strings probably need updating as well: I've been trying to keep them in sync.

@amunra
Copy link
Collaborator

amunra commented Mar 22, 2024

Clarification:

I had written feedback about reverting buffer -> buf changes.
I don't particularly care which variable name is used, so long as it's consistent with the rest of the examples in questdb-rs/examples as well.

@mtopolnik
Copy link
Contributor Author

I had a specific reason to use buf instead of buffer -- making the construct sender.flush(&mut buffer) a bit shorter, as I was going to use it in sentences whenever referring to flush. But in the end, I use it just once, so I'll revert to buffer.

@mtopolnik
Copy link
Contributor Author

cargo test --no-default-features doesn't compile on either main or this branch.

@amunra
Copy link
Collaborator

amunra commented Mar 23, 2024

Sorry. Correct. Need to enable at least webpki-roots.

@mtopolnik
Copy link
Contributor Author

test_tls_ca_webpki_and_os_roots is failing with

LineProtocolException: [-1] table: 77836915f4494765acbd56ec18f6dd7e, column: a; cast error from protocol type: TAG to column type: STRING

My guess is that there's something wrong with the isolation between tests. I suppose this test should create its own table, but it seems to already exist and there's a type clash on column a.

@mtopolnik
Copy link
Contributor Author

Working on the docs, I noticed that line_sender.hpp doesn't declare flush_and_keep_with_flags, so it seems you can't ensure a transactional flush from C++.

@jerrinot
Copy link
Contributor

I think the Ingress doc is missing a chapter on error handling. there is only this:

Finally, the client will keep retrying the request if it experiences errors.
You can configure the total time budget for retrying: retry_timeout (milliseconds, default 10 seconds)

A good understanding of error handling is critical for successful usage. Something like this: https://py-questdb-client.readthedocs.io/en/latest/sender.html#error-reporting Or here is Java: https://questdb.io/docs/clients/java_ilp/#error-handling

Copy link
Contributor

@jerrinot jerrinot left a comment

Choose a reason for hiding this comment

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

overall it's a pretty great improvement!
I see only two bigger things:

  1. consider deemphasizing the TCP transport. Move it from the Getting Started chapter (=the material for absolute beginners, it's OK to be opinionated there)
  2. explain error handling in the Rust Ingres docs. I think that's pretty important for production usage.

@bluestreak01 bluestreak01 merged commit 94b3890 into main Mar 26, 2024
@bluestreak01 bluestreak01 deleted the mt_improve-ingress-doc branch March 26, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incomplete ingres documentation Error in Getting Started example

6 participants