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

vcc: Internal type for the reserved 'default' symbol #4190

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

dridi
Copy link
Member

@dridi dridi commented Sep 19, 2024

A new DEFAULT type identifies symbols called 'default' and types that can have a default symbol carry a pseudo symbol. This approach can reconcile the overloaded default symbols that all share the generic type DEFAULT but carry their respective kinds.

This helps remove some of the special casing for default probes and backends whilst offering a sentinel DEFAULT type to safely deal with the special casing where it is actually needed, simplifying the code a wee bit in those areas.

Fixes #4177

A new DEFAULT type identifies symbols called 'default' and types that
can have a default symbol carry a pseudo symbol. This approach can
reconcile the overloaded default symbols that all share the generic type
DEFAULT but carry their respective kinds.

This helps remove some of the special casing for default probes and
backends whilst offering a sentinel DEFAULT type to safely deal with the
special casing where it is actually needed, simplifying the code a wee
bit in those areas.

Fixes varnishcache#4177
@nigoroll
Copy link
Member

I appreciate the fix, but as a vcc-noob I would be like to learn why a DEFAULT type is the better option to having multiple symbols named default with different types.
Do we not create a mess with type methods etc?

@dridi
Copy link
Member Author

dridi commented Sep 23, 2024

I appreciate the fix, but as a vcc-noob I would be like to learn why a DEFAULT type is the better option to having multiple symbols named default with different types.

When I started using Varnish it was perfectly legal to have multiple symbols of the same type, see for example 9962996 that introduced coverage for a corner case. The test case contains 3 distinct debug symbols.

It was a deliberate choice to only allow one symbol for a given name in bd30219 and as you can see from the explanation in the commit message that is the turning point where symbols became unique. The default symbol and its special meaning turned into a pretend BACKEND symbol that was either half wrong or half correct.

The introduction of a DEFAULT type here makes it completely wrong or completely correct.

As a side note on symbol uniqueness modulus default, another change happened afterwards where symbols became unique per namespace in #3344. A perfect segue into the next point you raise.

Do we not create a mess with type methods etc?

Yes but not really. Because default.method() becomes ambiguous depending on the underlying type, I'm more than willing to disallow calling methods or attributes directly from the default symbol.

@bsdphk
Copy link
Contributor

bsdphk commented Sep 23, 2024

I think I'm OK with this handling of it.

@nigoroll
Copy link
Member

I'm more than willing to disallow calling methods or attributes directly from the default symbol.

Not sure if this is a good idea, but under all circumstances something like

set bereq.backend = default;
set bereq.http.healthy = bereq.backend.healthy()

should continue to work.

@dridi
Copy link
Member Author

dridi commented Sep 23, 2024

This is fine because bereq.backend is a different symbol of the actual BACKEND type. This change does not affect other symbols of the types BACKEND and PROBE, but only the actual default pseudo-symbol.

Copy link
Member

@nigoroll nigoroll left a comment

Choose a reason for hiding this comment

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

I think this can be merged, phk agreed already

@dridi
Copy link
Member Author

dridi commented Sep 24, 2024

As long as there were questions or remarks I would still consider the review to be in progress. Between here and #4177 you raised interesting points.

@dridi dridi merged commit f2c2596 into varnishcache:master Sep 24, 2024
10 of 11 checks passed
@dridi dridi deleted the default_via branch September 24, 2024 07:25
dridi added a commit that referenced this pull request Sep 24, 2024
I shouldn't have picked the magic values from vmodtool.py in the first
place.

Refs #4190
@nigoroll
Copy link
Member

late correction:

have multiple symbols of the same type

I think you meant "multiple symbols of the name name (and different types)" and also read this, even though this is not was was written.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default backend cannot contain a .via backend
3 participants