feat(kv): label names to be unique #15647
Conversation
dearyhud
changed the title
dearyhud
force-pushed
the
feat/http-unique-label-names
branch
from
a3ae35f
to
e9c8929
Compare
dearyhud
force-pushed
the
feat/http-unique-label-names
branch
from
e9c8929
to
018b841
Compare
| @@ -85,7 +92,7 @@ func (s *Service) findLabelByID(ctx context.Context, tx Tx, id influxdb.ID) (*in | |||
|
|
|||
| func filterLabelsFn(filter influxdb.LabelFilter) func(l *influxdb.Label) bool { | |||
| return func(label *influxdb.Label) bool { | |||
| return (filter.Name == "" || (filter.Name == label.Name)) && | |||
| return (filter.Name == "" || (strings.EqualFold(filter.Name, label.Name))) && | |||
There was a problem hiding this comment.
is this change to maintain this with utf8 chars? what's the motivation behind this change?
There was a problem hiding this comment.
yeah. our CI linter also seemed to agree with the change.
I had changed things originally to deal w/ finding labels case insensitively like so
strings.ToLower(filter.Name) == strings.ToLower(label.Name) and CI called foul on that use.
|
|
||
| l.Name = strings.TrimSpace(l.Name) | ||
|
|
||
| if err := s.uniqueLabelName(ctx, tx, l); err != nil { |
There was a problem hiding this comment.
one thing we may want to do here is move the read for unqiueness outside the update. In cloud this would force a TX imagine, which seems like a bad idea for this. Moving this to a separate read before the update seems like a better idea 🤔. Also, why are we trimming white space on line 265? is that a behavior that we share across the kv implementations?
There was a problem hiding this comment.
re: transactions - I'm just leaving a note here since we chatted about this. This is an even larger issue that may not get addressed in this PR. TX is pretty heavily depended on unfortunately for reading/writing to kv buckets.
|
|
||
| k := make([]byte, influxdb.IDLength+len(l.Name)) | ||
| copy(k, orgID) | ||
| copy(k[influxdb.IDLength:], []byte(strings.ToLower((l.Name)))) |
There was a problem hiding this comment.
are we making names unique case insensitive now?
There was a problem hiding this comment.
Yup. label names should be unique & case agnostic. the UI currently handles all this stuff. feels like it's eagerly loading the labels and doing the label name validation in the browser...
for this change I added, the casing of the label names in the label bucket shouldn't matter & can be a mix of upper and lower case no problem. this system bucket is going to store the down-cased version of the label name, and we'll be using that to decide if a matching label name string already exists.
There was a problem hiding this comment.
excellent! still feels like a service concern, not necessarily a store concern 🤷♂, not even sure where that fits in here tbh
dearyhud
force-pushed
the
feat/http-unique-label-names
branch
from
6c7dccb
to
8bd8e08
Compare
Closes #15591
Describe your proposed changes here.