Skip to content

Conversation

@xiazhvera
Copy link
Contributor

@xiazhvera xiazhvera commented Apr 29, 2025

Issue #, if available:

Description of changes:
The pr depends on bug fixes of awslabs/aws-c-io#725 to make sure the streaming operation would terminated properly while the client is shutdown.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@xiazhvera xiazhvera changed the title [WIP]RR Streaming Operation Request-Response Streaming Operation May 7, 2025
@xiazhvera xiazhvera marked this pull request as ready for review May 7, 2025 22:40

/// The topic associated with this PUBLISH packet.
let topic: String
/// (Optional) User Properties, if there is no user property, the array is empty
Copy link
Contributor

Choose a reason for hiding this comment

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

Trivial: Should we allow this to be nil by making it optional ( [UserProperty]? ) ? Other optional properties are treated as optional so it feels a little off for this "Optional" to not be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the concern here. However, since Swift requires explicit validation before using optional values, using an empty array to represent "optional user properties" would simplify the code by avoiding the nil checks/unwrap.

}

/// Submit a request responds operation, throws CRTError if the operation failed
/// Submits a generic request to the request-response client, throws CRTError if the operation failed
Copy link
Contributor

Choose a reason for hiding this comment

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

Trivial: "Submits a generic request to the request-response client" -> "Initiates a request-response operation"

Copy link

@bretambrose bretambrose left a comment

Choose a reason for hiding this comment

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

Please no "ResponseResponse". If I did it in the C API it was a mistake there too, but let's not propagate it.

final public class ResponsePath: CStruct, @unchecked Sendable {
/// MQTT topic that a response may arrive on.
let topic: String
/// JSON path for finding correlation tokens within payloads that arrive on this path's topic.
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels misleading because it's labeled here and elsewhere as "JsonPath" but it's actually just the unique string used to correlate. Unsure what we should do from a naming the member perspective but I don't think we should say it's a "JSON path" in the description we provide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling it a "path" still feels accurate to me. Currently, the correlation token is just a top-level string, so the "path" is simply "clientToken" like in this example:

Response{
  "clientToken": "<token>"
}

But our current design leaves room for future responses where the token could be nested. For example, in the following case, the path might be something like "level1.level2.clientToken".

Response{
  "level1": {
    "level2" : {
        "clientToken": "<token>"
    }
  }
}

///
/// - Parameters:
/// - topic: MQTT topic that a response may arrive on.
/// - correlationTokenJsonPath: JSON path for finding correlation tokens within payloads that arrive on this path's topic.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor

@sbSteveK sbSteveK left a comment

Choose a reason for hiding this comment

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

Review trivial suggestions then ship

@xiazhvera xiazhvera merged commit 42e4916 into rr_operation Jun 6, 2025
29 of 30 checks passed
@xiazhvera xiazhvera deleted the rr_streaming branch June 6, 2025 16:31
@xiazhvera xiazhvera restored the rr_streaming branch June 6, 2025 16:31
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.

4 participants