ES Healthcheck v6 mapping compatibility #12714
Conversation
Some tests are still broken The sort method on the SavedObjectsClient isn't there yet
| return createNewConfig(); | ||
| } | ||
|
|
||
| // if the build number is still the template string (which it wil be in development) | ||
| // then we need to set it to the max interger. Otherwise we will set it to the build num | ||
| body._source.buildNum = config.get('pkg.buildNum'); | ||
| configSavedObject.attributes.buildNum = config.get('pkg.buildNum'); |
There was a problem hiding this comment.
We could use configSavedObject.get('buildNum') here
There was a problem hiding this comment.
I'm not seeing any get/set methods on the configSavedObject.
There was a problem hiding this comment.
Good point - that is on the client-side. Sometime in the future I should move the SavedObject to common and use for both client and server-side.
src/server/stats/stats.js
Outdated
| @@ -6,8 +6,8 @@ async function getStatsForType(savedObjectsClient, type) { | |||
| return { total }; | |||
| } | |||
|
|
|||
| export async function getStats(kibanaIndex, callAdminCluster) { | |||
| const savedObjectsClient = new SavedObjectsClient(kibanaIndex, callAdminCluster); | |||
| export async function getStats(kibanaIndex, mappings, callAdminCluster) { | |||
There was a problem hiding this comment.
Would be it easier to simply pass the savedObjectsClient here instead of all it's dependencies?
| const { callWithRequest } = req.server.plugins.elasticsearch.getCluster('admin'); | ||
| const callAdminCluster = (...args) => callWithRequest(req, ...args); | ||
| const savedObjectsClient = new SavedObjectsClient(config.get('kibana.index'), callAdminCluster); | ||
| const savedObjectsClient = req.getSavedObjectsClient(); |
|
|
||
| export default function (server, { mappings }) { | ||
| export default async function (server, { mappings }) { |
There was a problem hiding this comment.
Since server is just used to create the savedObjectsClient, how about updating these to simply accept the savedObjectsClient and we create it once in the health check?
There was a problem hiding this comment.
upgrade is using the server for server.log as well as server.config(), so I can't remove passing the server to these functions; however, I am now passing the savedObjectsClient to the upgrade function so it's only being constructed once.
| if (sortField) { | ||
| const value = { | ||
| order: sortOrder, | ||
| unmapped_type: get(mappings, `${type}.properties.${sortField}.type`) |
There was a problem hiding this comment.
For v6 mappings, this path would be doc.properties.${type}.properties.${sortField}.type
Could look something like this:
const v5MappingType = get(mappings, `${type}.properties.${sortField}.type`);
const v6MappingType = get(mappings, `doc.properties.${type}.properties.${sortField}.type`);
const mappingType = v5MappingType || v6MappingType;
const value = {
order: sortOrder,
unmapped_type: mappingType
};
There was a problem hiding this comment.
Are you sure these mapping paths will change for v6? As far as I can tell, these mappings are being specified by https://github.com/elastic/kibana/blob/master/src/core_plugins/kibana/mappings.json and I didn't see any changes to this for v6.
There was a problem hiding this comment.
You're right - found this when working on getting Kibana to work against ES master. Updating the mapping definition is definitely what caused it.
| if (sortField) { | ||
| const value = { | ||
| order: sortOrder, | ||
| unmapped_type: get(mappings, `${type}.properties.${sortField}.type`) |
There was a problem hiding this comment.
Is there any chance that sortField or type could have a . in the value? If so, then the second parameter of lodash get should be an array like [type, 'properties', sortField, 'type']
There was a problem hiding this comment.
Not currently because of https://www.elastic.co/guide/en/elasticsearch/reference/2.4/dots-in-names.html . However, I do see the benefit of not having to manually create this path string and prefer the approach that you've recommended as it's safe if Elasticsearch does decide to allow dots in field names.
* Beginning to update the healthcheck to use the SavedObjectsClient Some tests are still broken The sort method on the SavedObjectsClient isn't there yet * Adding sort to create_find_query * Fixing the tests * Fixing upgrade_config tests * Making the SavedObjectsClient be dependant on the mappings to enable sorting * Fixing disabled tests * Fiixng test wording * Passing the savedObjectsClient to the stats route handler * Passing the savedObjectsClient to upgradeConfig from migratConfig * Using array of keys with _.get instead of manual string concatenation
* Beginning to update the healthcheck to use the SavedObjectsClient Some tests are still broken The sort method on the SavedObjectsClient isn't there yet * Adding sort to create_find_query * Fixing the tests * Fixing upgrade_config tests * Making the SavedObjectsClient be dependant on the mappings to enable sorting * Fixing disabled tests * Fiixng test wording * Passing the savedObjectsClient to the stats route handler * Passing the savedObjectsClient to upgradeConfig from migratConfig * Using array of keys with _.get instead of manual string concatenation
|
Backported |
Resolves #12669