-
Notifications
You must be signed in to change notification settings - Fork 33
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
Conversation
Hey, thanks for pinging this change. I think this makes sense, and it will reduce the OPS tasks when they add more layers/sources. |
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 |
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 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. |
This is currently blocked by pelias/api#1419 and can be merged once that's resolved. |
872b900
to
eff4cec
Compare
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.
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? |
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
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.
eff4cec
to
81c5f5a
Compare
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. |
I would like to change the default value of targets.auto_discover from
false
totrue
.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.
I think this setting would be better off defaulting to
true
and that advanced users can set it tofalse
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