testing: update Go to 1.19 #5717

dfawley · 2022-10-12T16:57:02Z

RELEASE NOTES: none

new diffs

easwars · 2022-10-12T17:08:02Z

balancer/rls/config.go

+//   - within each entry:
+//   - must have at least one `Name`
+//   - must not have a `Name` with the `service` field unset or empty
+//   - within each `headers` entry:
+//   - must not have `required_match` set
+//   - must not have `key` unset or empty
+//   - across all `headers`, `constant_keys` and `extra_keys` fields:
+//   - must not have the same `key` specified twice
+//   - no `key` must be the empty string


Some of these are sub bullet points. How do we handle these with the new formatting?

Missed this. It looks really bad without a marker at the start of the line. I used /.

security/authorization/engine/engine.go

status/status.go

easwars · 2022-10-12T17:13:22Z

xds/internal/balancer/clusterresolver/configbuilder.go

+//	┌────────┐
+//	│priority│
+//	└┬──────┬┘
+//	 │      │
+//
 // ┌──────────▼─┐  ┌─▼──────────┐
 // │cluster_impl│  │cluster_impl│
 // └──────┬─────┘  └─────┬──────┘
-//        │              │
+//
+//	│              │
+//


ASCII art fiasco.

easwars · 2022-10-12T17:13:50Z

xds/internal/balancer/priority/balancer_priority.go

 //   - For any of the following cases:
-//     - If balancer is not started (not built), this is either a new child with
-//       high priority, or a new builder for an existing child.
-//     - If balancer is Connecting and has non-nil initTimer (meaning it
-//       transitioned from Ready or Idle to connecting, not from TF, so we
-//       should give it init-time to connect).
-//     - If balancer is READY or IDLE
-//     - If this is the lowest priority
+//   - If balancer is not started (not built), this is either a new child with
+//     high priority, or a new builder for an existing child.
+//   - If balancer is Connecting and has non-nil initTimer (meaning it
+//     transitioned from Ready or Idle to connecting, not from TF, so we
+//     should give it init-time to connect).
+//   - If balancer is READY or IDLE
+//   - If this is the lowest priority
 //   - do the following:
-//     - if this is not the old childInUse, override picker so old picker is no
-//       longer used.
-//     - switch to it (because all higher priorities are neither new or Ready)
-//     - forward the new addresses and config
+//   - if this is not the old childInUse, override picker so old picker is no
+//     longer used.
+//   - switch to it (because all higher priorities are neither new or Ready)
+//   - forward the new addresses and config


Same here, sub bullet points need to be handled.

Fixed as a code block

easwars · 2022-10-12T17:14:39Z

xds/internal/xdsclient/xdsresource/matcher.go

+//   - A match is better if it’s matching pattern type is better
+//   - Exact match > suffix match > prefix match > universal match
+//   - If two matches are of the same pattern type, the longer match is better
+//   - This is to compare the length of the matching pattern, e.g. “*ABCDE” >
+//     “*ABC”


Sub-bullets

easwars · 2022-10-12T18:03:39Z

balancer/grpclb/grpclb.go

+//   - It's OK to consider IDLE as Connecting. SubConns never stay in IDLE,
+//     they start to connect immediately. But there's a race between the overall
+//     state is reported, and when the new SubConn state arrives. And SubConns
+//     never go back to IDLE.


This one was a sub-bullet earlier. Sorry, missed this in the last pass.

easwars · 2022-10-12T18:07:49Z

internal/balancergroup/balancergroup.go

+//   - service config update
+//   - address update
+//   - subConn state change
+//   - find the corresponding balancer and forward


This was sub-bullet

easwars · 2022-10-12T18:08:07Z

internal/balancergroup/balancergroup.go

-//     - sub-pickers are sent to an aggregator provided by the parent, which
+//   - new/remove SubConn
+//   - picker update and health states change
+//   - sub-pickers are sent to an aggregator provided by the parent, which


This was a sub-bullet

easwars · 2022-10-12T18:25:31Z

xds/internal/balancer/ringhash/ringhash.go

 // - the new state is TransientFailure, to update the error message
 //   - it's possible that this is a noop, but sending an extra update is easier
-//   than comparing errors
+//     than comparing errors
+//
 // - the aggregated state is changed
 //   - the same picker will be sent again, but this update may trigger a re-pick
-//   for some RPCs.
+//     for some RPCs.


Interesting that gofmt didnt reformat this as single list. Should we just rewrite this comment as:

// UpdateSubConnState updates the per-SubConn state stored in the ring, and also // the aggregated state. // // It triggers an update to cc when: // - the new state is TransientFailure to update the error message. // It's possible that this is a noop, but sending an extra update is easier // than comparing errors // - the aggregated state has changed. The same picker will be sent again, // but this update may trigger a re-pick for some RPCs.

I honestly don't understand this. I changed it to use tabs which is how code blocks get formatted.

easwars · 2022-10-12T18:28:58Z

balancer/rls/config.go

+//	 / routeLookupConfig:
+//	   / grpc_keybuilders field:
+//	     / must have at least one entry
+//	     / must not have two entries with the same `Name`
+//	     / within each entry:
+//	       / must have at least one `Name`
+//	       / must not have a `Name` with the `service` field unset or empty
+//	       / within each `headers` entry:
+//	         / must not have `required_match` set
+//	         / must not have `key` unset or empty
+//	       / across all `headers`, `constant_keys` and `extra_keys` fields:
+//	         / must not have the same `key` specified twice
+//	         / no `key` must be the empty string
+//	/ `lookup_service` field must be set and must parse as a target URI
+//	/ if `max_age` > 5m, it should be set to 5 minutes
+//	/ if `stale_age` > `max_age`, ignore it
+//	/ if `stale_age` is set, then `max_age` must also be set
+//	/ ignore `valid_targets` field
+//	/ `cache_size_bytes` field must have a value greater than 0, and if its
+//	  value is greater than 5M, we cap it at 5M
+//
 // - routeLookupChannelServiceConfig:
 //   - if specified, must parse as valid service config
+//
 // - childPolicy:
 //   - must find a valid child policy with a valid config
+//
 // - childPolicyConfigTargetFieldName:
 //   - must be set and non-empty


Should we just reformat this as a code block. The / make the whole thing harder to read. And it does not keep routeLookupConfig, routeLookupChannelServiceConfig, childPolicy and childPolicyConfigTargetFieldName at the same indent level.

This is a code block; the slash prevents it from being detected as a list instead. But I found another workaround which I will try here and elsewhere.

easwars · 2022-10-13T01:37:42Z

balancer/rls/config.go

+//	- `lookup_service` field must be set and must parse as a target URI
+//	- if `max_age` > 5m, it should be set to 5 minutes
+//	- if `stale_age` > `max_age`, ignore it
+//	- if `stale_age` is set, then `max_age` must also be set
+//	- ignore `valid_targets` field
+//	- `cache_size_bytes` field must have a value greater than 0, and if its
+//	  value is greater than 5M, we cap it at 5M


Sorry, these should be sub-bullets inside of routeLookupConfig. They are fields in the route lookup config. This should be the last one.

Whoops; my mistake. Fixed

testing: update Go to 1.19

b6d2646

dfawley added the Type: Testing label Oct 12, 2022

dfawley added this to the 1.51 Release milestone Oct 12, 2022

dfawley requested review from easwars and zasweq October 12, 2022 16:57

dfawley assigned easwars and zasweq Oct 12, 2022

gofmt

60e30ec

easwars previously approved these changes Oct 12, 2022

View reviewed changes

regenerate pb.gos with go1.19

f43eb33

easwars reviewed Oct 12, 2022

View reviewed changes

dfawley added 2 commits October 12, 2022 10:42

fix comments

bd15195

more comments

e39236f

easwars reviewed Oct 12, 2022

View reviewed changes

easwars assigned dfawley and unassigned easwars Oct 12, 2022

more fixing

a1f6c85

easwars approved these changes Oct 13, 2022

View reviewed changes

easwars assigned dfawley and unassigned dfawley and zasweq Oct 13, 2022

fix config.go

dcb733c

easwars approved these changes Oct 17, 2022

View reviewed changes

dfawley merged commit 778860e into grpc:master Oct 17, 2022

dfawley deleted the go119 branch October 17, 2022 22:04

github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testing: update Go to 1.19 #5717

testing: update Go to 1.19 #5717

dfawley commented Oct 12, 2022

easwars Oct 12, 2022

dfawley Oct 12, 2022

easwars Oct 12, 2022

dfawley Oct 12, 2022

easwars Oct 12, 2022

dfawley Oct 12, 2022

easwars Oct 12, 2022

dfawley Oct 12, 2022

easwars Oct 12, 2022

dfawley Oct 12, 2022

easwars Oct 12, 2022

dfawley Oct 12, 2022

easwars Oct 12, 2022

dfawley Oct 12, 2022

easwars Oct 12, 2022

dfawley Oct 12, 2022

easwars Oct 12, 2022

dfawley Oct 12, 2022

easwars Oct 13, 2022

dfawley Oct 17, 2022

testing: update Go to 1.19 #5717

testing: update Go to 1.19 #5717

Conversation

dfawley commented Oct 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment