Migrate to Opaque API for protobuf#2192
Conversation
🔬 Benchmark DetailsTo validate the performance impact of the Opaque API migration, a dedicated benchmark was created simulating a large Benchmark Codepackage benchmarks
import (
"fmt"
"testing"
"github.com/ydb-platform/ydb-go-genproto/protos/Ydb"
"google.golang.org/protobuf/proto"
)
func buildOpaqueResultSet(columns, rows int) *Ydb.ResultSet {
cols := make([]*Ydb.Column, 0, columns)
for i := 0; i < columns; i++ {
cols = append(cols, Ydb.Column_builder{
Name: fmt.Sprintf("col_%d", i),
Type: Ydb.Type_builder{
TypeId: Ydb.Type_UTF8.Enum(),
}.Build(),
}.Build())
}
rs := make([]*Ydb.Value, 0, rows)
for r := 0; r < rows; r++ {
items := make([]*Ydb.Value, 0, columns)
for c := 0; c < columns; c++ {
items = append(items, Ydb.Value_builder{
TextValue: proto.String(fmt.Sprintf("value_%d_%d", r, c)),
}.Build())
}
rs = append(rs, Ydb.Value_builder{Items: items}.Build())
}
return Ydb.ResultSet_builder{
Columns: cols,
Rows: rs,
}.Build()
}
func BenchmarkResultSet_FullRead(b *testing.B) {
rs := buildOpaqueResultSet(100, 1000)
data, _ := proto.Marshal(rs)
b.ResetTimer()
b.ReportAllocs()
for i := 0; i < b.N; i++ {
target := &Ydb.ResultSet{}
_ = proto.Unmarshal(data, target)
for _, row := range target.GetRows() {
for _, item := range row.GetItems() {
_ = item.GetTextValue()
}
}
}
}
func BenchmarkResultSet_PartialRead(b *testing.B) {
rs := buildOpaqueResultSet(100, 1000)
data, _ = proto.Marshal(rs)
b.ResetTimer()
b.ReportAllocs()
for i := 0; i < b.N; i++ {
target := &Ydb.ResultSet{}
_ = proto.Unmarshal(data, target)
for _, row := range target.GetRows() {
if len(row.GetItems()) > 0 {
_ = row.GetItems()[0].GetTextValue()
}
}
}
} |
| golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 | ||
| google.golang.org/grpc v1.78.0 | ||
| google.golang.org/protobuf v1.36.10 | ||
| google.golang.org/grpc v1.81.1 |
There was a problem hiding this comment.
По ряду причин, желательно ограничить версией v1.79.3
| Ydb_Discovery_V1.DiscoveryService_WhoAmI_FullMethodName, | ||
| &Ydb_Discovery.WhoAmIRequest{}, | ||
| &Ydb_Discovery.WhoAmIResponse{}, | ||
| gomock.Any(), |
There was a problem hiding this comment.
Не пойму, откуда взялся ещё один аргумент. Наверное, из-за обновления grpc либы (которое надо ограничить, написал в отдельном комменте).
| func (r *WriteRequest) Size() int { | ||
| if mess, err := r.toProto(); err == nil { | ||
| size := proto.Size(mess.WriteRequest) + writeRequestClientMessageSize | ||
| size := proto.Size(mess) + writeRequestClientMessageSize |
There was a problem hiding this comment.
Смущает, что был mess.WriteRequest, а теперь замеряем размер mess целиком. Но возможно, что это и правильно, надо разбираться.
|
Analysis performed by claude, claude-opus-4-6. |
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ❌ 2 major issue(s) found
Critical issues
No critical issues found.
Major issues
- Major | High:
columnFamiliessemantics changed from replace to append —table/options/options.go:256,265 - Major | Medium: PR branch diverged before
feat(topic): support topic metrics_level (#2181)landed on master. Merging will produce conflicts in rawtopic files; careless resolution could drop the MetricsLevel feature —internal/grpcwrapper/rawtopic/create_topic.go,alter_topic.go,describe_topic.go
Other findings
- Minor | High:
log/topic.go:1306-1309— Pre-existing bug carried over: afterdata.GetMessages()[i] = nil, callingSetMetadataItems(nil)on the nil element (line 1309) will panic. The defer block (lines 1314-1315) has the same issue. This exists on master too, but the migration preserves the bug rather than fixing it. - Minor | High:
internal/table/scanner/scanner.go— AllvalueTypeError()calls now passnil, nilorcur, nilinstead of the actual oneof variant types, degrading error messages from e.g."BoolValue; want Int32Value"to"nil; want nil"or"Ydb.Value; want nil". - Minor | Medium:
internal/tx/control_test.go:140-147— Tests usefmt.Sprintf("%+v")to compare protobuf messages instead ofproto.Equal(). The Opaque API does not guarantee stable%+voutput across versions, making these assertions fragile.
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
| } | ||
| rd.SetAddColumnFamilies(append(rd.GetAddColumnFamilies(), families...)) |
There was a problem hiding this comment.
Severity: Major
Confidence: High
Semantic change from replace to append. On master, ApplyAlterTableOption did:
d.AddColumnFamilies = make([]*Ydb_Table.ColumnFamily, len(cf))This replaced the entire slice. The new code does:
rd.SetAddColumnFamilies(append(rd.GetAddColumnFamilies(), families...))This appends to existing families. The same change applies to ApplyCreateTableOption at line 265.
If a user calls WithColumnFamilies() (or WithAddColumnFamilies()) multiple times on the same descriptor, the old code would keep only the last call's families (overwrite), while the new code accumulates all calls' families.
Notably, columnFamilies was the only option type that used replace semantics — all others (e.g., column, columnMeta, primaryKey) already used append. So the new behavior is arguably more consistent, but it is a semantic change to a public API.
Suggested fix: if the original replace behavior was intentional, use rd.SetAddColumnFamilies(families) (no append). If append is desired, this is fine but should be documented as a behavior change.
| @@ -1303,16 +1303,16 @@ func (s lazyProtoStringifer) String() string { | |||
| storage := make([]messDataType, len(data.GetMessages())) | |||
| for i := range data.GetMessages() { | |||
| storage[i].Data = data.GetMessages()[i].GetData() | |||
| data.Messages[i] = nil | |||
| data.GetMessages()[i] = nil | |||
There was a problem hiding this comment.
Severity: Minor
Confidence: High
Pre-existing bug carried over from master: after setting data.GetMessages()[i] = nil here, line 1309 calls data.GetMessages()[i].SetMetadataItems(nil) on the nil element, which will panic (setter methods panic on nil receiver in the Opaque API). The defer block (lines 1314-1315) also calls SetData and SetMetadataItems on the nil element.
The same bug exists on master (data.Messages[i] = nil followed by data.Messages[i].MetadataItems = nil), so this is not a regression. But since this code is being touched, it would be a good opportunity to fix the ordering:
for i := range data.GetMessages() {
msg := data.GetMessages()[i]
storage[i].Data = msg.GetData()
storage[i].Metadata = msg.GetMetadataItems()
msg.SetData(nil)
msg.SetMetadataItems(nil)
}
defer func() {
for i := range data.GetMessages() {
msg := data.GetMessages()[i]
msg.SetData(storage[i].Data)
msg.SetMetadataItems(storage[i].Metadata)
}
}()This avoids nil-ing the element pointer, instead clearing the fields directly on the message object.
| return | ||
| cur, ok := s.stack.currentValue().(*Ydb.Value) | ||
| if !ok { | ||
| s.valueTypeError(nil, nil) |
There was a problem hiding this comment.
Severity: Minor
Confidence: High
All valueTypeError() calls throughout this file now pass nil, nil or cur, nil instead of the actual oneof variant types. In the old code, the type assertions provided informative type names for error messages (e.g., "*Ydb.Value_BoolValue; want *Ydb.Value_Int32Value"). With nil, nil, the error messages degrade to "nil; want nil" which is unhelpful for debugging.
Consider passing the WhichValue() case constant or a descriptive string to improve diagnostics. For example:
if cur.WhichValue() != Ydb.Value_BoolValue_case {
s.valueTypeError(cur.WhichValue(), Ydb.Value_BoolValue_case)
}(This would require updating the valueTypeError signature to accept the case enum type.)
|
Analysis performed by claude, claude-opus-4-6. |
This commit resolves all merge conflicts arising from the Opaque API migration and addresses specific code review feedback. Key changes: - Resolved all merge conflicts with the latest master branch. - Pinned google.golang.org/grpc to v1.79.3. - Restored 'replace' semantics for columnFamilies in table/options (reverted accidental 'append'). - Improved error diagnostics in internal/table/scanner by passing actual WhichValue() types instead of nil. - Replaced fragile fmt.Sprintf comparisons with proto.Equal in internal/tx/control_test.go. - Fixed gomock expectations in internal/conn/conn_test.go by adding missing gomock.Any() for variadic opts. - Fixed MetricsLevel handling in rawtopic (create/describe) using proper builder getters/setters.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2192 +/- ##
==========================================
- Coverage 77.97% 77.79% -0.18%
==========================================
Files 453 453
Lines 46185 46275 +90
==========================================
- Hits 36011 35998 -13
- Misses 8096 8172 +76
- Partials 2078 2105 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kartunovv
changed the title
kartunovv
changed the title
3 participants
Migration to Opaque API for protobuf
What changed
Migrated from Hybrid API to Opaque API for protobuf generated code. This is the modern approach recommended by the protobuf team.
Key changes:
Performance impact
Benchmark results for large ResultSet (100 columns × 1000 rows):
Full benchmark results and comparison charts attached
Testing