Consider how multiple hosts can be specified when establishing a connection

[elprans]

Currently, we only allow a single host/port pair to be passed to the connect() call. To make building HA setups around the binding(s) easier, we should allow passing a list of addresses to be tried in order. On its own, this would satisfy simple HA setups with static hostlists, either direct server addresses or load-balancing proxy lists.

For reference, PostgreSQL's libpq documentation on how to specify multiple hosts is here.

Their approach is hybrid: host and port are separate arguments and both may contain lists. If host and port are both lists, they must be of the same size, if host is a list and port is not, the specified port is applied to all hosts.

The upside of this approach is that it keeps things simple in the very common case of a single host/port, and the downside is that separate host and port lists are harder to specify and manage.

An alternative approach is to remove the port argument and instead allow passing host as either a string, a single (named) tuple, or a list of (named) tuples.

All of this applies to how the credentials file is structured, since we want to keep the resemblance with connect() call signatures.

[1st1]

Here's the proposed credentials schema by @tailhook https://gist.github.com/tailhook/bb64fc0fa67a5981876d3646cfc326c7

[tailhook]

1. I don't think host: ['alpha.example.org', 'beta.example.org'], port: [5656, 5657] is good format in 2020
2. I don't think we should forgo the load-balancing. Most likely we want it for read-only replicas for 95% users.
3. I don't think credentials file must contain anything about HA setup. I see host/port in credentials file as a shortcut for a simple command-line connections, for staging and local installations. I would expect even CI that applies migration to contain edgedb --credentials-file=xxx --host=host1.example.org override (so this is seen in the CI log).

With the above in mind, I would propose the following:

connect(credentials_file=..., ha_config=FailoverList(['alpha1.example.org', 'beta.example.org']))

With the following semantics:

1. ha_config conflicts with dsn, host and port (i.e. trigger ValueError if both are specified)
2. ha_config overrides host/port in credentials_file
3. ha_config is a subclass. More examples of future extensions:
   a) FailoverList.from_srv('_edgedb.example.org')
   b) LoadBalancing(['alpha1.example.org', 'beta.example.org'])
   c) SomeSmartLoadBalancer(...) (some / ideas)
4. ha_config can never refer to a unix socket

The couple of points:

1. A class is easy to deprecate, while setting in credentials_file will likely stay there forever
2. Sometimes we need other settings. Examples: failover conditions and timeouts. Subclass can have correct way of setting this kind of things whether they are keyword arguments or methods (builder/fluent pattern). Subclass also works great for the documentation.

While ha_config may still be read from config sometimes, it's not obvious that we should implement a canonical way for that. Some proprietary project would like to configure that as:

ha_config=FailoverList([f'edgedb{n}.{clustername}' for n in range(num)])

instead of having full configuration. Some open-source projects, like django-based ones have pythonic config anyway, so users can create ha_config with programmatic API there. We may do the canonical way for some of the most used techniques later as we see them used in the wild.

Obviously, we can bikeshed on the names.

[elprans]

I don't think host: ['alpha.example.org', 'beta.example.org'], port: [5656, 5657] is good format in 2020

For the record, I don't either.

ha_config

So, this is basically a "host selector" kind of API. Something like:

class HostSelector:
    def next_host(self) -> Tuple[str, int] // host, port
        ... 
    
    def invalidate_host(self, host: str, port: int) -> None:
        ...

I think that could work.

[1st1]

I don't think host: ['alpha.example.org', 'beta.example.org'], port: [5656, 5657] is good format in 2020

I agree. FWIW I only saw Elvis proposing [(host, port), ...] format, which is different.

---

Regarding

Obviously, we can bikeshed on the names.

and

ha_config overrides host/port in credentials_file

I assume it would also override regularly passed host/port? E.g. would this be a legal call connect(my_host, my_port, ha_config=...)?

Other than that, I'm not a big fan of the "ha_config" name -- it only allows to configure a certain tiny aspect and it doesn't suggest that it can override something. Which makes me wonder why do we need to stuff all this functionality into one connect() function? In high level network service frameworks like Finagle things like that are achieved via "client builders", which give you great flexibility to compose desired behaviors.

[tailhook]

So, this is basically a "host selector" kind of API. Something like:

I would make it produce a subclass of a connection pool:

pool: Pool = ha_config.create_pool(credentials_file=..., **kwargs)

I think there might be much more than just next_host and invalidate_host. Some systems like this use response latency for each request to choose host to send next query to.

But anyway we should not make any public API out of ha_config except for configuration itself and be able to experiment with it.

Which makes me wonder why do we need to stuff all this functionality into one connect() function? In high level network service frameworks like Finagle things like that are achieved via "client builders", which give you great flexibility to compose desired behaviors.

We already do that in Rust, because it has no keyword arguments. In python it's common to have keyword arguments for many things, instead of builder/fluent. In Python community there is often backlash for builder/fluent.

So, yes if it wasn't clear I propose a builder only for ha_config specifically:

ha_config = FailoverList([...]).retries(5).failover_on_timeout(true)

But simpler things like tutorials and ad-hoc scripts will still look simpler like connect(credentials_file=file) or connect(dsn=url). This is Pythonic enough and still very powerful.

Other than that, I'm not a big fan of the "ha_config" name

Well, I'm not saying this is a perfect name. The idea is that until you don't need high-availability (HA), you don't specify this name, and once you have, you specify everything that may influence availability in the class that build HA config. This includes multi-level timeouts, failover conditions, load-balancing way.

This interoperates very much with settings of the connection pool, but they are still pretty orthogonal. So, I don't think this should be pool_class or something like that.

But I'm not married to the specific name. I've thought of network_config but that already quiestions of why host/port are separate options.

---

Another thing I've just noticed that ha_config is mostly useful for create_async_pool rather than for connect. And we can make config other way around:

 FailoverList([...])
.retries(5)
.failover_on_timeout(true)
.create_pool(credentials_file=..., **other_settings)

This is very Rustish way, but I would prefer it be keyword argument, because discoverability. Here is the difference between usages:

create_async_pool(**kwargs)  # (1)
create_async_pool(ha_config=config, **kwargs)  # (2)
config.create_pool()  # (3)

The difference between (1) and (2) is much smaller than between (1) and (3). And when you create a pool for the first time it's easier to remember how to create HA (production) config by looking at the function signature. Otherwise, we need to explicitly show example with config.create_pool() in create_async_pool documentation which is kinda out of place.

And it's also easier to make a config that toggles keyword argument (production vs dev env).

[1st1]

I actually quite like what @elprans proposed in edgedb/edgedb#1683. IMO the approach allows to implement what @tailhook and I have in mind with a builder API.