feat(codegen): fill URI/endpoint-bound members for the CRaC warm-up call and warm sync/async clients independently - #7183
Conversation
…all and warm sync/async clients independently
| try { | ||
| task.run(); | ||
| return true; | ||
| } catch (RuntimeException | LinkageError e) { |
There was a problem hiding this comment.
This behaviour is same ad before, as part of this PR moved it to a function so that this can be resused.
| } | ||
|
|
||
| /** | ||
| * The required input members the warm-up call must give a dummy value. A member needs one when it is bound to the |
There was a problem hiding this comment.
The required input members the warm-up call must give a dummy value.
Nit: grammar is incorrect (?) reads funny.
|
How did we discover this? Was this just a manual check to see which services have code generated a warmup call and which ones didnt? Did we do the same check after this PR to check if there are any remaining gaps? |
| static boolean isArnMember(MemberModel member) { | ||
| String name = member.getName(); | ||
| return member.getContextParam() != null | ||
| && (name.endsWith("Arn") || name.endsWith("ARN")); |
There was a problem hiding this comment.
Do you know how accurate this endsWith signal is? This heuristic seems like an ok signal, but I'm worried about edge cases that this might miss. For example S3 Bucket can accept an ARN so it wont end with "arn".
I think we can use the model's endpoint ruleset for a more authoritative source of whether a member is an arn:
{
"fn": "aws.parseArn",
"argv": [
{
"ref": "Bucket"
}
],
"assign": "bucketArn"
}To be completely honest I don't know if this edge case is even applicable here. For all intents and purposes a heuristic endsWtih("Arn") might be good enough. Can you just verify?
There was a problem hiding this comment.
In WarmUp providers we do canned HTTP calls and the calls are stubbed. Thus the above is the best effort to make the call successful. If it still fails, we have warm-up tests implemented as part of #7188, this will catch the failure even before the release.
Can you just verify?
In the current case the only ARN based parameter that was getting picked was KVSArn, and this was able to generate the ARN parameter as intended.
I am removing the ARN handling from this PR since this case is very specific to one service and I am planning to do it via customization to keep codegen code simple.
This was discovered as a part of #7188 . This PR added generic test to make sure all services modules have the wamrUp provider and also make sure the warmUp API executes with out any issues.
Yes I did the local changes to the PR-7188 and it passed , once I merge this PR the build check of 7188 should pass. |
febc92d
into
feature/master/crac_auto_priming_support
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
2 participants
Motivation and Context
The CRaC warm-up selector did not select an operation that has a required member bound to the URI path or to an endpoint context param. These members do not accept null. Some services have this member on every candidate operation. For these services, the selector found no operation. Thus the SDK did not prime the marshalling path or the signing path before the checkpoint.
Also,
warmUp()warmed the sync client and the async client in one flow. If one client failed, the other client did not warm.Modifications
WarmUpOperationSelector): Accept an operation when all of its required URI/endpoint-bound members are of typeString. TheallDummyMembersAreFillablegate does this check. Add themembersRequiringDummyValuehelper.WarmUpProviderSpec): Emit a dummy value ("warmup") for each required bound member. Move the client build into the sharedclientBuilderhelper and theaddServiceSpecificOptionshelper (bearer token, endpoint-discovery disable). The sync path and the async path share these helpers.SdkWarmUpProvider.warmUp(),TargetedWarmUpInvoker): Route both paths through the sharedWarmUpDiscovery.runSafely(description, task)helper. The sync client and the async client warm on their own. If one client fails, the SDK writes a warn log and continues to warm the other client.Modules that always need each fix
Fix A. Dummy value for a required URI-path or endpoint-bound member. These modules have a required member that the marshaller rejects on null. They now get
.member("warmup").These modules need a dummy value for a required URI-path member:
apigatewaymanagementapi,iotjobsdataplane,lexruntime,lexruntimev2,quicksight,sagemakerruntime,sagemakerfeaturestoreruntime,workmailmessageflow,ebs,chimesdkmeetings,amplifyuibuilder,marketplacedeployment,eksauth,geomaps,transcribestreaming.One module needs a dummy value for an endpoint context param:
s3controlAccountIdendpoint context param (validated only as a host label, so"warmup"works)Fix A. Bearer-token auth.
codecatalyst.tokenProvider(StaticTokenProvider.create(() -> "warmup-dummy-token"))so it does not fall through to the profile/SSO token chain and fail.Fix B. Endpoint-discovery WARN suppression.
timestreamwrite.endpointDiscoveryEnabled(false)to stop the client constructor logging a WARN about the endpoint override disabling discoverytimestreamqueryTesting
License