Skip to content

Conversation

travisdowns
Copy link
Contributor

Neither are used (they are never read). metric_help is set in one place and hostname is never set. They are vestigial.

Officially deprecated them and remove them at API_LEVEL 9 and above.

Use some macros to suppress warnings about their use in implicitly generated constructors.

@avikivity
Copy link
Member

/cc @amnonh

To create an API_LEVEL, you need to update configure.py, compatibility.md, CMakeStupidName.txt

@travisdowns
Copy link
Contributor Author

To create an API_LEVEL, you need to update configure.py, compatibility.md, CMakeStupidName.txt

My goal wasn't to create an API level, but mark this as for deletion in the next API level. After re-reading compat.md I'm to sure if that's the right approach: perhaps deprecation (which is addressed there separately from "breaking changes") does not need to be tied to an API level, just a long-enough notice period.

If it does need an API level, I imagine we don't want to create a new API level changes for each minor change like this, rather we would schedule/batch several small changes into the one API level bump: that was my attempt here: schedule removal for API_LEVEL=9, and when some bigger changes comes along that pushes us to actually introduce that level all the associated changes are noted.

@avikivity
Copy link
Member

Yes, if you want to [[deprecate]] something, just do it. It can be garbage collected later.

API_LEVELs are reserved for truly breaking changes.

@travisdowns travisdowns force-pushed the td-deprecate-unused-prom-config branch from 3518f19 to 94ede7e Compare October 9, 2025 18:30
@travisdowns
Copy link
Contributor Author

Thanks @avikivity, I've removed the API_LEVEL stuff and just deprecated these two fields with a 2027 suggested removal date.


// Helpers for ignoring deprecation warnings in code that has to deal with
// deprecated APIs, e.g., constructors/etc for structs with deprecated fields.
#define BEGIN_IGNORE_DEPRECATIONS \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please qualify with SEASTAR_INTERNAL so we don't intrude on the global macro namespace in a visible header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Neither are used (they are never read). metric_help is set in one place
and hostname is never set. They are vestigial.

Officially deprecated them and remove them at API_LEVEL 9 and above.

Use some macros to suppress warnings about their use in implicitly
generated constructors.
@travisdowns travisdowns force-pushed the td-deprecate-unused-prom-config branch from 03e7c0c to fbdc314 Compare October 15, 2025 13:12
@avikivity
Copy link
Member

Still need review from @amnonh

@avikivity avikivity requested a review from amnonh October 16, 2025 10:56
Copy link
Contributor

@amnonh amnonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can definitely be removed; it's a leftover from older times, where collectd was supported and not all metrics had help lines.

@travisdowns
Copy link
Contributor Author

travisdowns commented Oct 17, 2025

where collectd was supported and not all metrics had help lines.

Is collectd no longer supported? I would love to that backend as would @StephanDollberg in #2586.

For now can I just unconditionally delete these?

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