Skip to content
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

build,ui: add support for protobuf WKTs #14396

Merged
merged 3 commits into from
Mar 27, 2017
Merged

build,ui: add support for protobuf WKTs #14396

merged 3 commits into from
Mar 27, 2017

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Mar 27, 2017

See individual commits.


This change is Reviewable

@tamird tamird requested a review from mrtracy March 27, 2017 21:35
@mrtracy
Copy link
Contributor

mrtracy commented Mar 27, 2017

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 58 of 58 files at r3, 1 of 1 files at r5, 60 of 60 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.


build/protobuf.mk, line 51 at r3 (raw file):

# Map protobuf includes to the Go package containing the generated Go code.
MAPPINGS :=
MAPPINGS := $(MAPPINGS)Mgoogle/api/annotations.proto=$(GRPC_GATEWAY_GOOGLEAPIS_PACKAGE)/google/api,

Is that weird M necessary after $(MAPPINGS)? Just looks off to me, assuming this all works finen


pkg/server/status/status.proto, line 40 at r3 (raw file):

  repeated StoreStatus store_statuses = 6 [(gogoproto.nullable) = false];
  repeated string args = 7;
  repeated string env = 8;

Why did these change?


pkg/ui/src/containers/events.spec.tsx, line 42 at r3 (raw file):

      events: [
        {
          target_id: Long.fromString("1"),

well this was a bizarre choice


pkg/ui/src/containers/metricsDataProvider.spec.tsx, line 45 at r3 (raw file):

        name: "test.metric.1",
        sources: sources,
        downsampler: protos.cockroach.ts.tspb.TimeSeriesQueryAggregator.AVG,

sick


pkg/ui/src/util/api.spec.ts, line 377 at r3 (raw file):

          }).finish();
          return {
            body: encodedResponse.buffer.slice(encodedResponse.byteOffset, encodedResponse.byteOffset + encodedResponse.byteLength),

Couldn't at least the slice part be made into a common method?

interface EncodedResponseBuffer {
   buffer: string;
   byteOffset: number;
   byteLength: number;
}

export function trimResponse(response: EncodedResponseBuffer) {
   return response.buffer.slice(response.byteOffset, response.byteOffset + response.byteLength)
}

Or is there a subtle difference between these?


Comments from Reviewable

@mrtracy
Copy link
Contributor

mrtracy commented Mar 27, 2017

LGTM mostly, but you are right that the buffer slicing is annoying and i'm wondering if it shouldn't be extracted to a common method. There may be no good way to get the "encode().finish()" bit due to the various types involved, but the slicing can almost certainly be extracted and does involve some difficult-to-grok parameters.

@tamird
Copy link
Contributor Author

tamird commented Mar 27, 2017

Extracted!


Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


build/protobuf.mk, line 51 at r3 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Is that weird M necessary after $(MAPPINGS)? Just looks off to me, assuming this all works finen

Yeah, it's necessary. Note that it was there before this change as well.


pkg/server/status/status.proto, line 40 at r3 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Why did these change?

Because nullable = false doesn't do anything for repeat primitives in proto3. The commit message calls this out.


pkg/ui/src/containers/events.spec.tsx, line 42 at r3 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

well this was a bizarre choice

Yup!


pkg/ui/src/containers/metricsDataProvider.spec.tsx, line 45 at r3 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

sick

🎊


pkg/ui/src/util/api.spec.ts, line 377 at r3 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Couldn't at least the slice part be made into a common method?

interface EncodedResponseBuffer {
   buffer: string;
   byteOffset: number;
   byteLength: number;
}

export function trimResponse(response: EncodedResponseBuffer) {
   return response.buffer.slice(response.byteOffset, response.byteOffset + response.byteLength)
}

Or is there a subtle difference between these?

No, you're absolutely right. Pulled out a util function.


Comments from Reviewable

@mrtracy
Copy link
Contributor

mrtracy commented Mar 27, 2017

:lgtm:


Reviewed 1 of 59 files at r4, 14 of 14 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/ui/src/components/graphs.tsx, line 361 at r7 (raw file):

}

type formattedDatum = {

why this syntax instead of interface formattedDatum?


Comments from Reviewable

tamird added 3 commits March 27, 2017 19:10
- Address warnings in server/status/status.proto
- Remove overzealous `any`s

Closes #6616
@tamird tamird merged commit 2a341b9 into cockroachdb:master Mar 27, 2017
@tamird tamird deleted the protobuf-polish branch March 27, 2017 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants