Make array elements order to be stable and follow yaml Node #59
Conversation
bitomaxsp
changed the title
bitomaxsp
force-pushed
the
test-allof-order
branch
from
a5a0336
to
f24a517
Compare
Codecov ReportBase: 99.77% // Head: 99.77% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
- Coverage 99.77% 99.77% -0.01%
==========================================
Files 136 136
Lines 7851 7838 -13
==========================================
- Hits 7833 7820 -13
Misses 18 18
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| buildSchema := func(sch lowmodel.ValueReference[*base.SchemaProxy], bChan chan *SchemaProxy) { | ||
| p := &SchemaProxy{schema: &lowmodel.NodeReference[*base.SchemaProxy]{ | ||
| // for every item, build schema sync since it is an array | ||
| buildSchema := func(sch lowmodel.ValueReference[*base.SchemaProxy]) *SchemaProxy { |
There was a problem hiding this comment.
There is a very real chance this behavior could result in a performance hit on the library, I understand why it's been moved to a sync model, I just want to ensure that before changing it permanently, it's not going to slow down applications with seriously large schemas.
There was a problem hiding this comment.
Would you be able to A/B test this change in a benchmark with some of the sample specs in test_specs.
the k8s or Stripe APIs are generally a good benchmark to check.
| }, | ||
| v: *res, | ||
| } | ||
| // We need to build arrays sequentially so call it in the loop instead of goroutines |
There was a problem hiding this comment.
Like my previous comment, to be sure this doesn't cost us performance (speed is essential for my use cases), I want to make sure this move to a sync model does not cost us anything. I don't think it will - but I need to be sure. Could you add some A/B benchmarks to ensure we're not slowing down?
There was a problem hiding this comment.
Hi @daveshanley . Sorry, i was on vacation.I tried doing benchmark, but on both (stripe and k8s) specs lib finds circular refs. (I followed BenchmarkNewDocument example for benchmarking)
I assume I need to wait for #64 to be merged. Or there is another approach: fork k8s spec and fix circular there then base test on it. What do you prefer ?
There was a problem hiding this comment.
(Just jumping in to note that the Stripe API spec that's in the repo does actually contain some invalid infinite circular references, so that will still fail to load until I get around to adding the options to treat these as warnings rather than errors. The k8s spec should work correctly once our PR is completed.)
There was a problem hiding this comment.
@BenjaminNolan Do you have any ETA on PR being merged?
There was a problem hiding this comment.
Hey just want to jump in here, I have been working on an alternate PR that retains the async building nature but still retains the original order so shouldn't have an impact on performance. I am happy to continue with that and get that PR up ASAP which should reduce the need for benchmarking as we won't be changing the performance characteristics
There was a problem hiding this comment.
@TristanSpeakEasy Fantastic. When can you merge #66?
|
Closed in favor of #66 |
Test to confirm #58