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

b4.4 deprecation fix: fix warnings about using reconnect instead of interval #1480

Open
wants to merge 1 commit into
base: b4.4
Choose a base branch
from

Conversation

baallan
Copy link
Collaborator

@baallan baallan commented Oct 25, 2024

the following bit of sed came handy in updating scripts:

sed -i -E -e 's%(^prdcr_add) (.*) interval=%\1 \2 reconnect=%' *
sed -i -e 's/reconnect=/reconnect_interval=/' *

@baallan baallan added this to the v4.4.5 milestone Oct 25, 2024
@baallan
Copy link
Collaborator Author

baallan commented Oct 25, 2024

need to make a cherrypick version of this for 4.5

@tom95858
Copy link
Collaborator

@baallan, I think @valleydlr would prefer if the examples used "reconnect_interval" instead of "reconnect". @nichamon would you please confirm that we support either "reconnect" or "reconnect_interval" in b4.4?

@baallan
Copy link
Collaborator Author

baallan commented Oct 28, 2024

This patch is formulated based on the deprecation message seen. If we want something else, we need to update the deprecation message. @nichamon let me know if there's a final conclusion for 4.4 and if it will differ for main.

@tom95858
Copy link
Collaborator

tom95858 commented Nov 2, 2024

@baallan, there has been discussion about renaming 'interval' --> 'sample_interval'. @nichamon, It seems like we should have this discussion in the UG meeting on Monday. The advocates of this change argue that there would be symmetry in the naming. By that logic, however, name would become 'producer_name', etc... In other words, a context free grammar. I see that as fundamentally reductive, but let's have the discussion.

@baallan
Copy link
Collaborator Author

baallan commented Nov 3, 2024

I would be in favor of minimizing breaking changes to the spelling of existing things other than the ambiguous 'interval' until an ldms 5.0 where we might decide to use full spelling of words and more context clue prefixes instead of all the arbitrary letter omissions. If somewhere in 4.x we want to introduce a bunch of aliasing tables (prdcr|producer) which do not break any existing configuration scripts, that's fine by me.

@baallan baallan force-pushed the static-test-update-reconnect branch from c785c21 to 9625dd9 Compare November 5, 2024 16:36
@baallan
Copy link
Collaborator Author

baallan commented Nov 5, 2024

@nichamon this is ready to merge after some patch that adds reconnect_interval is merged.

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.

2 participants