-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
upgrade to use google protobuf #9190
base: main
Are you sure you want to change the base?
Conversation
a71470b
to
8367466
Compare
8367466
to
b4c3780
Compare
42b8fa8
to
5f181e3
Compare
08167c7
to
1953bbe
Compare
bc55364
to
1470a02
Compare
@@ -128,6 +128,7 @@ func (p *proposals) Done(key uint64, err error) { | |||
type RaftServer struct { | |||
m sync.RWMutex | |||
node *Node | |||
pb.RaftServer |
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.
We need to ensure that we implement the pb.RaftServer
interface instead embedding it like this
@@ -79,6 +79,7 @@ type Server struct { | |||
blockCommitsOn *sync.Map | |||
|
|||
checkpointPerGroup map[uint32]uint64 | |||
pb.UnimplementedZeroServer |
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.
probably the same issue here
m := jsonpb.Marshaler{EmitDefaults: true} | ||
var jsonState bytes.Buffer | ||
if err := m.Marshal(&jsonState, ms); err != nil { | ||
m := protojson.MarshalOptions{EmitUnpopulated: true} |
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.
could we get some understanding of EmitDefaults
vs EmitUnpopulated
?
@@ -102,7 +101,9 @@ var ( | |||
) | |||
|
|||
// Server implements protos.DgraphServer | |||
type Server struct{} | |||
type Server struct { | |||
api.DgraphServer |
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.
same problem here too
@@ -31,8 +29,8 @@ const ( | |||
// If these credentials are missing the default credentials will be used. | |||
type MinioCredentials struct { | |||
AccessKey string | |||
SecretKey pb.Sensitive | |||
SessionToken pb.Sensitive | |||
SecretKey string |
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 start printing these sensitive data into logs, we need to figure out a fix for this.
@@ -101,7 +101,7 @@ func TestZeroWithAllRoutesTLSWithTLSClient(t *testing.T) { | |||
} | |||
|
|||
body := readResponseBody(t, do) | |||
if !strings.Contains(string(body), test.response) { | |||
if !strings.Contains(strings.ReplaceAll(string(body), " ", ""), test.response) { |
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.
why are we calling ReplaceAll
here?
size := proto.Size(list) | ||
|
||
// Ensure the size is non-negative and fits within uint64 bounds | ||
if size < 0 { |
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.
it should be an assert
@@ -78,6 +78,7 @@ func Init(ps *badger.DB) { | |||
// grpcWorker struct implements the gRPC server interface. | |||
type grpcWorker struct { | |||
sync.Mutex | |||
pb.WorkerServer |
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.
same here
@@ -419,7 +419,7 @@ func (p ParsedKey) ToBackupKey() *pb.BackupKey { | |||
key.Attr = attr | |||
key.Uid = p.Uid | |||
key.StartUid = p.StartUid | |||
key.Term = p.Term | |||
key.Term = []byte(p.Term) |
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 may affect performance
@@ -455,7 +455,7 @@ func FromBackupKey(backupKey *pb.BackupKey) []byte { | |||
case pb.BackupKey_DATA: | |||
key = DataKey(attr, backupKey.Uid) | |||
case pb.BackupKey_INDEX: | |||
key = IndexKey(attr, backupKey.Term) | |||
key = IndexKey(attr, string(backupKey.Term)) |
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.
same
inspired from #7313