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.

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

sdk: Lower client cache log severities #162

Merged
merged 2 commits into from
Aug 10, 2023
Merged

sdk: Lower client cache log severities #162

merged 2 commits into from
Aug 10, 2023

Conversation

kim
Copy link
Contributor

@kim kim commented Aug 9, 2023

Description of Changes

In particular, avoid logging entire rows above trace.

API

  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

In particular, avoid logging entire rows above `trace`.
@kim kim requested a review from gefjon August 9, 2023 10:45
@kim
Copy link
Contributor Author

kim commented Aug 9, 2023

On the fence re error! in this module: it's not really an error if we continue. Demote to warn! maybe?

Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

I agree with @Centril that the conditionals on log level clog the code in a way I don't love. I don't see much reason to offer the higher-level logs which don't include the row; it seems to me that these logs are useful only when debugging row representation and database state, so just knowing that some mysterious row was inserted/deleted/maintained seems unlikely to be useful.

I'd also appreciate being consistent about the log level: handle_row_update should log at trace the same as the other methods.

crates/sdk/src/client_cache.rs Outdated Show resolved Hide resolved
crates/sdk/src/client_cache.rs Outdated Show resolved Hide resolved
@gefjon
Copy link
Contributor

gefjon commented Aug 9, 2023

On the fence re error! in this module: it's not really an error if we continue. Demote to warn! maybe?

I mean, any of the error logs denote a serious problem which will cause the client to behave wrong, most likely due to outdated module bindings. I think logging at error is reasonable. Honestly, likely all of these cases should exit the process.

@kim
Copy link
Contributor Author

kim commented Aug 9, 2023

Honestly, likely all of these cases should exit the process.

Seriously? That does not seem like a viable option when using the sdk in a server context. Rather, I'd like to have some way of getting notified if / when some error state is encountered, so I can just throw away the client and re-initialize it.

@kim
Copy link
Contributor Author

kim commented Aug 9, 2023

most likely due to outdated module bindings

Hm ok, if that can be inferred with some certainty, process::exit might be reasonable. But wouldn't some kind of ALPN be preferable?

@gefjon
Copy link
Contributor

gefjon commented Aug 9, 2023

most likely due to outdated module bindings

Hm ok, if that can be inferred with some certainty, process::exit might be reasonable. But wouldn't some kind of ALPN be preferable?

As things stand now, both the module and the client must share a single canonical schema. With the tools we have now, recovering from a mismatch involves regenerating the client's module_bindings and recompiling the client. In the future I hope we'll have a better story (in particular this will be important once we get migrations), but for now there's not much we can do.

The only alternative to a schema mismatch I can think of is receiving corrupted data, which given that we communicate over TCP is also probably a world-ending (or process-ending) problem.

@kim
Copy link
Contributor Author

kim commented Aug 9, 2023

Okay, so, if these are all fatal errors, how can I know they happened and terminate my process accordingly?

@kim
Copy link
Contributor Author

kim commented Aug 9, 2023

We can move this discussion elsewhere if you prefer, but for what I'm doing I think I'd prefer those cases to cause a panic (perhaps feature-gated?). Except for connection errors, in which case it would be great to be able to reconnect.

@kim kim requested a review from gefjon August 10, 2023 07:22
@Centril
Copy link
Contributor

Centril commented Aug 10, 2023

I'll defer this review to @gefjon since they are the domain expert here.

@kim kim merged commit 5bdd0b9 into master Aug 10, 2023
5 checks passed
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.

3 participants