-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use configSavedObject.get('buildNum')
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing any get
/set
methods on the configSavedObject
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
export default function (server, { mappings }) { | ||
export default async function (server, { mappings }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right - found this when working on getting Kibana to work against ES master. Updating the mapping definition is definitely what caused it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for taking on this issue @kobelb!
if (sortField) { | ||
const value = { | ||
order: sortOrder, | ||
unmapped_type: get(mappings, `${type}.properties.${sortField}.type`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* 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