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

Gjoranv/node info #11840

Merged
merged 4 commits into from Jan 20, 2020
Merged

Gjoranv/node info #11840

merged 4 commits into from Jan 20, 2020

Conversation

gjoranv
Copy link
Member

@gjoranv gjoranv commented Jan 17, 2020

This makes the format from /applicationmetrics/v1/values equal to the future /metrics/v2/values api.

@gjoranv gjoranv requested a review from olaaun January 17, 2020 14:41
@bratseth
Copy link
Member

I don't think you should rename a field as that will break any clients of this?

@gjoranv
Copy link
Member Author

gjoranv commented Jan 17, 2020

Probably true, because we still don't bundle def-files with the fat model. I will add it as a new field with a TODO to remove the old one when the oldest hosted version is past using it.

@hmusum Can you verify if this is necessary? This is effectively removing and adding a field without a default value.

@hmusum
Copy link
Member

hmusum commented Jan 18, 2020

You should not rename a field like this. See https://docs.vespa.ai/documentation/cloudconfig/configapi-dev.html#def-compatibility. Essentially, you can't remove a field without a default value.

I guess you could add a default value for the field you want to remove, wait until all users have the config with default value and then remove it.

Another caveat: You cannot remove a field if there are config overrides referring to the field you want to remove.

@bratseth
Copy link
Member

I commented on changing the web service API:
"Expose hostname, and rename nodeId -> role."

But yes, you also cannot rename (that is, remove) a config value.

@gjoranv
Copy link
Member Author

gjoranv commented Jan 20, 2020

I thought more about this, and it is ok because there are no user configs to this config def. The config model always produces the value for the correct version. I have done this before without issues. But I will play it safe and add a temporary default before renaming.

Regarding the web API, this is only an internal API, not public.

@bratseth
Copy link
Member

It's not an internal API: https://docs.vespa.ai/documentation/reference/metrics.html

@gjoranv
Copy link
Member Author

gjoranv commented Jan 20, 2020

That's the old API, where this field does not exist.

- Add a default value because the model    does not produce the
  value anymore.
- This should not be necessary. The field can safely be removed
  because the config model of older versions always adds a value
  for this field.
@gjoranv gjoranv merged commit 9ba0df9 into master Jan 20, 2020
@gjoranv gjoranv deleted the gjoranv/node-info branch January 20, 2020 11:02
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.

None yet

4 participants