-
Notifications
You must be signed in to change notification settings - Fork 113
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
RSDK-9870: Check for schema changes all the time. Allow for FTDC to consume maps. #4756
Conversation
ftdc/custom_format.go
Outdated
@@ -136,6 +138,11 @@ func isNumeric(kind reflect.Kind) bool { | |||
} | |||
|
|||
func flattenStruct(item reflect.Value) ([]float32, error) { | |||
_, b, c := flattenStructNew(item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new flattenStructNew
does everything flattenStruct
does and more (gets the fields for a schema). To help test correctness of the new code, I've started this PR where everything funnels into the new code.
ftdc/custom_format.go
Outdated
return fields, numbers, nil | ||
} | ||
|
||
func getFieldsForStruct(item reflect.Value) ([]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new flattenStructNew
does everything getFieldsForStruct[Old]
does and more (gets the data values). To help test correctness of the new code, I've started this PR where everything funnels into the new code.
ftdc/ftdc.go
Outdated
@@ -356,6 +357,131 @@ func (ftdc *FTDC) constructDatum() datum { | |||
return datum | |||
} | |||
|
|||
func schemaDiff(prev []string, curr []string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I use this anymore
ftdc/ftdc.go
Outdated
return !slices.Equal(prev, curr) | ||
} | ||
|
||
func walk(data map[string]any, inputSchema *schema) (*schema, []float32, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new method that combines getting a schema and data values.
ftdc/ftdc.go
Outdated
return inputSchema, values, nil | ||
} | ||
|
||
func (ftdc *FTDC) writeDatumNew(datum datum) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will become the new method to use.
ftdc/ftdc.go
Outdated
// The first iteration when `currSchema` is nil, it's guaranteed that the generations will not | ||
// be equal. In the happy path where the schema hasn't changed, the `walk` function is | ||
// guaranteed to return the same schema object. | ||
if datum.generationID != ftdc.outputGenerationID || ftdc.currSchema != newSchema { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think generations will go away entirely. But I'll save that for a later commit in this PR
ftdc/ftdc_test.go
Outdated
@@ -124,6 +211,7 @@ func (badStatser badStatser) Stats() any { | |||
} | |||
|
|||
func TestRemoveBadStatser(t *testing.T) { | |||
t.Skip("It is now legal to change schemas without a corresponding `Add`/`Remove` statster call.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This asserts on a behavior that is no longer true
ftdc/ftdc_test.go
Outdated
@@ -248,6 +336,7 @@ func TestNestedStructs(t *testing.T) { | |||
// TestStatsWriterContinuesOnSchemaError asserts that "schema errors" are handled by removing the | |||
// violating statser, but otherwise FTDC keeps going. | |||
func TestStatsWriterContinuesOnSchemaError(t *testing.T) { | |||
t.Skip("It is now legal to change schemas without a corresponding `Add`/`Remove` statster call.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This asserts on a behavior that is no longer true
No description provided.