Skip to content

Conversation

clintonpi
Copy link
Contributor

Motivation:

Implement the Health service defined in Protos/upstream/grpc/health/v1/health.proto.

Modifications:

  • Implement the "check" and "watch" methods for the service.
  • Implement providers (public APIs) to interact with the service.
  • Add tests.

Result:

The Health service will be available for use in grpc-swift.

Motivation:

This is needed to compile the tests and their dependencies into a test suite.

Modifications:

- Add a new target "grpcHealthTests" with its necessary dependencies to Package@swift-6.swift.

Result:

The tests will be able to access their dependencies.
Motivation:

The tests for the Health service need access to the client and server stubs for the service. Only the server stub was generated but its visibility is `internal`. Therefore, the tests cannot access it.

Modifications:

- Adjust the function that generates the stubs to:
 - make the visibility of the server stub `package`.
 - generate the client stub too.

Result:

The tests will now have access to both the client and server stubs.
Motivation:

Implement the Health service defined in Protos/upstream/grpc/health/v1/health.proto.

Modifications:

- Implement the check and watch methods for the service.
- Implement providers (public APIs) to interact with the service.
- Add tests.

Result:

The Health service will be available for use in grpc-swift.
.grpcInProcessTransport
],
path: "Tests/Services/HealthTests",
swiftSettings: [.swiftLanguageVersion(.v5), .enableUpcomingFeature("ExistentialAny")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be .v6


/// A coupled Health service and provider.
@available(macOS 15.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
public struct Health {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Sendable


/// Provides handlers to interact with a Health service.
@available(macOS 15.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
public struct HealthProvider: Sendable {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we nest this within Health?

extension Health {
  public struct Provider {
    // ...
  }
}


/// A registerable RPC service to probe whether a server is able to handle RPCs.
@available(macOS 15.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
public final class HealthService: RegistrableRPCService {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we nest this within Health?

extension Health {
  public struct Service {
    // ...
  }
}


/// A registerable RPC service to probe whether a server is able to handle RPCs.
@available(macOS 15.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
public final class HealthService: RegistrableRPCService {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this can be a struct and should be Sendable

Comment on lines 196 to 205
var responseStreamIterator = response.messages.makeAsyncIterator()

// Since responseStreamIterator.next() will never be nil (as the "watch" response stream
// is always open), the iteration cannot be based on when responseStreamIterator.next()
// is nil. Else, the iteration infinitely awaits and the test never finishes. Hence, it is
// based on the expected number of statuses to be received.
for _ in 0 ..< statusesToBeSent.count {
try await continuation.yield(responseStreamIterator.next()!.status)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need a continuation here, we can just get the first N elements from the response stream and collect them into an array and then return that from the RPC:

try await response.messages.prefix(N).reduce(into: []) { $0.append($1) }

Breaking that down:

// Get an async sequence which only returns the first N messages from `response.messages`
let prefix = response.messages.prefix(N) 

// Wait for the prefix sequence to end (i.e. collect all N elements) and store them in an array:
let reduced = prefix.reduce(into: []) { elements, nextElement in 
  elements.append(nextElement)
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can then reduce this test to:

try await withThrowingTaskGroup(of: [Status].self) { group in
  let concurrentStreams = 2
  for _ in 0 ..< concurrentStreams {
    group.addTask {
      try await watch()
    }
  }

  for status in statuses {
    // update status
  }

  for try await next in group {
    XCTAssertEqual(next, expected)
  }
}


func testCheckOnKnownService() async throws {
try await withHealthClient { (healthClient, healthProvider) in
let testServiceDescriptor = ServiceDescriptor(package: "test.package", service: "TestService")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than spelling out the descriptor every time we can add an extension within this file to ServiceDescriptor, e.g.:

extension ServiceDescriptor {
  fileprivate static let echo = ServiceDescriptor(package: "echo", service: "Echo")
}

///
/// - Parameters:
/// - from: The base status.
package init(from status: ServingStatus) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a 'value preserving type conversion' so should be:

  package init(_ status: ServingStatus) {

see: https://www.swift.org/documentation/api-design-guidelines/#type-conversion

Comment on lines 50 to 53
switch status.value {
case .serving: self = .serving
case .notServing: self = .notServing
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer each line doing one thing only:

Suggested change
switch status.value {
case .serving: self = .serving
case .notServing: self = .notServing
}
switch status.value {
case .serving:
self = .serving
case .notServing:
self = .notServing
}

@glbrntt glbrntt added the version/v2 Relates to v2 label Jul 23, 2024
@clintonpi clintonpi requested a review from glbrntt July 29, 2024 11:57
.grpcInProcessTransport
],
path: "Tests/Services/HealthTests",
swiftSettings: [.swiftLanguageVersion(.v6), .enableUpcomingFeature("ExistentialAny")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll need to be (see also #1995):

Suggested change
swiftSettings: [.swiftLanguageVersion(.v6), .enableUpcomingFeature("ExistentialAny")]
swiftSettings: [._swiftLanguageMode(.v6), .enableUpcomingFeature("ExistentialAny")]

/// A coupled Health service and provider.
@available(macOS 15.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
public struct Health: Sendable {
private let internalHealthService = InternalHealthService()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to store this here.

/// Updates the status of a service in the Health service.
public func updateStatus(
_ status: ServingStatus,
ofService service: ServiceDescriptor
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: forService would follow the shape of the API that Dictionary has (which is updateValue(_:forKey:))

Suggested change
ofService service: ServiceDescriptor
forService service: ServiceDescriptor

import GRPCCore

@available(macOS 15.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
internal final class InternalHealthService: Grpc_Health_V1_HealthServiceProtocol {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to call this InternalHealthService, HealthService is fine.

Comment on lines 97 to 99
storage[service, default: ServiceState(status: .serviceUnknown)]
.addContinuation(continuation)
continuation.yield(storage[service]!.currentStatus)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid the repeated hashing by yielding the value in addContinuation

fileprivate mutating func updateStatus(
_ status: Grpc_Health_V1_HealthCheckResponse.ServingStatus
) {
if self.currentStatus != status {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can early exit here:

guard status != self.currentStatus else { return }

...


let statusesToBeSent: [ServingStatus] = [.serving, .notServing, .serving]

healthProvider.updateStatus(statusesToBeSent[0], ofService: testServiceDescriptor)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment as to why we send this status before starting the RPC because that's quite important.

Comment on lines 156 to 159
var message = Grpc_Health_V1_HealthCheckRequest()
message.service = testServiceDescriptor.fullyQualifiedService

let immutableMessage = message
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just do:

let message = Grpc_Health_V1_HealthCheckRequest.with { 
  $0.service = ...
}

.serving,
]

try await withThrowingTaskGroup(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this test needs a task group, we should be able to do everything we need within watch:

healthClient.watch(...) { response in 
  var iterator = response.messages.makeAsyncIterator()
  // wait for 'serviceUnknown'
  // send status updates
  // read updates
}

public struct Health: Sendable {
private let internalHealthService = InternalHealthService()

/// A registerable RPC service to probe whether a server is able to handle RPCs.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// An implementation of the 'grpc.health.v1.Health' service.

/// A registerable RPC service to probe whether a server is able to handle RPCs.
public let service: Health.Service

/// Provides handlers to interact with the coupled Health service.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// Provides status updates to the health service.

@available(macOS 15.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
extension InternalHealthService {
extension HealthService {
/// The state of the Health service.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to document this

extension HealthService {
/// The state of the Health service.
private struct State: Sendable {
/// A locked value box of `["service name": ServiceState]`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't tell us anything new other than the key is the service name. You might want to say something like "The state of each service keyed by the fully qualified service name."

continuation.yield(self.currentStatus)
}

fileprivate init(status: Grpc_Health_V1_HealthCheckResponse.ServingStatus) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably makes sense to default this to "unknown"

Comment on lines 19 to 20
/// - `ServingStatus.serving` indicates that a service is healthy.
/// - `ServingStatus.notServing` indicates that a service is unhealthy.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// - `ServingStatus.serving` indicates that a service is healthy.
/// - `ServingStatus.notServing` indicates that a service is unhealthy.
/// - ``ServingStatus/serving`` indicates that a service is healthy.
/// - ``ServingStatus/notServing`` indicates that a service is unhealthy.

Comment on lines 70 to 71
$0.service =
ServiceDescriptor(package: "does.not", service: "Exist").fullyQualifiedService
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No point creating a ServiceDescriptor to immediately throw it away. All we want is a string for a service which doesn't exist.

Suggested change
$0.service =
ServiceDescriptor(package: "does.not", service: "Exist").fullyQualifiedService
$0.service = "does.not/Exist"

Comment on lines 47 to 52
extension ServiceDescriptor {
/// The descriptor for a server.
///
/// An unspecified service name refers to the server.
public static let server = ServiceDescriptor(package: "", service: "")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed

@clintonpi clintonpi requested a review from glbrntt July 31, 2024 16:30
import GRPCCore

@available(macOS 15.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
internal final class HealthService: Grpc_Health_V1_HealthServiceProtocol {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be possible for this to be a struct

@clintonpi clintonpi requested a review from glbrntt August 2, 2024 09:43
@glbrntt glbrntt requested a review from gjcairo August 2, 2024 15:35
@clintonpi clintonpi requested a review from gjcairo August 5, 2024 14:46
.grpcInProcessTransport
],
path: "Tests/Services/HealthTests",
swiftSettings: [._swiftLanguageMode(.v6), .enableUpcomingFeature("ExistentialAny")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the health module and health tests to use explicit imports like in Gus' PR #2003?

I'm happy for this to be done in a separate PR as well (actually it probably needs to be done separately because Gus needs to do the imports on the code-gen too).

@gjcairo gjcairo merged commit 8a72c8d into grpc:main Aug 7, 2024
@clintonpi clintonpi deleted the health-service branch August 7, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

version/v2 Relates to v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants