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

default targets.auto_discover=true #127

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Conversation

missinglink
Copy link
Member

@missinglink missinglink commented Mar 26, 2020

I would like to change the default value of targets.auto_discover from false to true.

It seems like in the majority of cases having auto_discover=true is preferable, the only case where this setting is not preferable is when running a very large index (like planet-wide) using only canonical sources (like Geocode Earth does).

There is a small startup delay of a couple seconds in those 'enterprise-level' installations of Pelias, and the concern is that the server would announce 'ready' to a load-balancer before it loaded its sources/layers from the DB.

note: canonical sources/layers are used until the auto_discover request returns, so the server can still respond to traffic

I think this setting would be better off defaulting to true and that advanced users can set it to false rather than the inverse?
It's really easy for developers to miss this setting when importing custom data, so this would reduce the bug reports.

Thoughts/feels?

cc/ @pelias/contributors

@Joxit
Copy link
Member

Joxit commented Mar 26, 2020

Hey, thanks for pinging this change.

I think this makes sense, and it will reduce the OPS tasks when they add more layers/sources.

@orangejulius
Copy link
Member

One complication is that setting this value to true will cause the unit tests in the API to fail if there isn't a local elasticsearch instance available: pelias/api#1419

@orangejulius
Copy link
Member

I happened to need to test this setting on a larger cluster today. While there was around 15 seconds of 100% CPU usage on a 4 core t3.xlarge instance, the CPU impact on a larger cluster was not even noticeable.

So we should definitely consider defaulting this to true, as it would reduce confusion quite a bit, and allow us to remove some fairly scary warnings/advice from our documentation.

@missinglink
Copy link
Member Author

This is currently blocked by pelias/api#1419 and can be merged once that's resolved.

orangejulius added a commit to pelias/api that referenced this pull request Apr 1, 2021
This change resolves #1419 by
ensuring all unit tests will run with the `api.targets.auto_discover`
property set to false, even if the value of that property in the
`pelias-config` defaults or `~/pelias.json` is set to true.

There were two changes required to achieve this:

First, expand upon the work in #1431
that added an empty `pelias.json` file for use in the unit tests. Its
now no longer empty and includes setting `auto_discover` to false.

Second, one unit test still ended up calling Elasticsearch with that
change. It was a test for the `TypeMapping` module itself, and it was
overriding `pelias-config` with its own. Now with a test-specific
`pelias.json` file, that code can simply be removed, and the unit tests
can use the unmodified `pelias-config` library.

I tested these changes against pelias/config#127
to make sure that PR won't break anything, and I believe everything is
good to go.
@orangejulius
Copy link
Member

Okay, I've rebased this old PR to make sure it's up to date, and that went smoothly.

After pelias/api#1521, I think this will be ok to merge.

It isn't currently, but I think we should mark this as a breaking change. It should only affect the API, and the API should soon be protected against any breakage, but doing that will give us a better opportunity to test and make sure. @missinglink what do you think?

orangejulius added a commit to pelias/api that referenced this pull request Apr 8, 2021
This change resolves #1419 by
ensuring all unit tests will run with the `api.targets.auto_discover`
property set to false, even if the value of that property in the
`pelias-config` defaults or `~/pelias.json` is set to true.

There were two changes required to achieve this:

First, expand upon the work in #1431
that added an empty `pelias.json` file for use in the unit tests. Its
now no longer empty and includes setting `auto_discover` to false.

Second, one unit test still ended up calling Elasticsearch with that
change. It was a test for the `TypeMapping` module itself, and it was
overriding `pelias-config` with its own. Now with a test-specific
`pelias.json` file, that code can simply be removed, and the unit tests
can use the unmodified `pelias-config` library.

I tested these changes against pelias/config#127
to make sure that PR won't break anything, and I believe everything is
good to go.

Closes #1419
orangejulius added a commit to pelias/api that referenced this pull request Apr 9, 2021
This change resolves #1419 by
ensuring all unit tests will run with the `api.targets.auto_discover`
property set to false, even if the value of that property in the
`pelias-config` defaults or `~/pelias.json` is set to true.

There were two changes required to achieve this:

First, expand upon the work in #1431
that added an empty `pelias.json` file for use in the unit tests. Its
now no longer empty and includes setting `auto_discover` to false.

Second, one unit test still ended up calling Elasticsearch with that
change. It was a test for the `TypeMapping` module itself, and it was
overriding `pelias-config` with its own. Now with a test-specific
`pelias.json` file, that code can simply be removed, and the unit tests
can use the unmodified `pelias-config` library.

I tested these changes against pelias/config#127
to make sure that PR won't break anything, and I believe everything is
good to go.

Closes #1419
BREAKING CHANGE: Change the default value of
`api.targets.auto_discover`, which [automatically detects sources and
layers from Elasticsearch](https://github.com/pelias/api#custom-sources-and-layers), to true.

This setting is the generally recommended value and is required for
Pelias users importing their own custom data.
@orangejulius
Copy link
Member

I updated the commit message so that this will be released as a new major version, giving us a bit more control of how we roll this out.

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