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

Improve error logging #109

Closed
wants to merge 1 commit into from
Closed

Improve error logging #109

wants to merge 1 commit into from

Conversation

rukai
Copy link
Member

@rukai rukai commented Jul 20, 2021

While trying to get shotover to connect to redis on my local machine I ran into a few issues that were impossible to investigate due to error logging.
So this PR fixes those cases to be properly logged.
The issues were:

  • Specifying a redis instance as part of a cluster that is not configured to run as a cluster.
  • Connecting to a redis instance that does not exist.
  • Attempting to load a config file that does not exist.

The config/topology.yaml seems to be specified in a previous? format as it needed to be changed to run on my machine.

@XA21X
Copy link
Member

XA21X commented Jul 20, 2021

The config/topology.yaml seems to be specified in a previous? format

Yeah, I've been told we used to use a proper Redis client instead of rolling our own.
I've opportunistically re-added support for these full URIs in #101.

I'm not sure if we should keep /config, as it's not really a working example, especially without documentation.
The basic_driver_tests spins up Dockerised clusters to supplement the example Shotover config (see /examples).


I hit most of those cases for missing error handling when I started. :')
I've also fixed some of them in a similar way in my PR.

@XA21X
Copy link
Member

XA21X commented Jul 20, 2021

PR looks good to me. Probably best to wait for @benbromhead to review and merge.

@rukai
Copy link
Member Author

rukai commented Jul 20, 2021

Hmmm, whats the benefit of readding support for those URL's, surely the less things we have to parse the better?

I'm all for removing /config.
In that case we should also change the defaults on --config-file and --topology-file, although i'm not sure what exactly they should be changed to.
Maybe just config.yaml and topology.yaml?

We should also rename all the config.yaml's in examples/ to topology.yaml because its super confusing how it is atm.

@XA21X
Copy link
Member

XA21X commented Jul 20, 2021

whats the benefit of readding support for those URL's

That's a good point. It has negative value right now so I will remove it. It was done while I was still trying to figure out the scope e.g. possibility of per-server authentication (or TLS) options, and I also thought it would be nice to get the existing inputs working before I started on a new feature, even though it was already broken.

we should also change the defaults on --config-file and --topology-file

Question is, should we expect the project root to be a somewhat usable working directory for running the application?
In the (managed service) Docker container, those files are located in /opt/shotover-proxy/config/.

On second thought, it might be preferable to leave the files and add some docs instead. 🤔

We should also rename all the config.yaml's in examples/

I didn't even realise they were named incorrectly. Definitely.

@rukai rukai mentioned this pull request Jul 22, 2021
@rukai
Copy link
Member Author

rukai commented Jul 22, 2021

Replaced by this PR which has access to CI due to the branch being on shotover/shotover-proxy instead of my fork #117

@rukai rukai closed this Jul 22, 2021
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.

2 participants