feat(codegen): add WarmUpOperationSelector to pick the CRaC warm-up operation#7135
feat(codegen): add WarmUpOperationSelector to pick the CRaC warm-up operation#7135joviegas wants to merge 2 commits into
Conversation
joviegas
force-pushed
the
joviegas/warmup_op_selector
branch
5 times, most recently
from
e2bce8e to
ff16b58
Compare
joviegas
force-pushed
the
joviegas/warmup_op_selector
branch
from
ff16b58 to
8a81efb
Compare
alextwoods
left a comment
alextwoods
left a comment
There was a problem hiding this comment.
It looks like the PR description doesn't quite match the actual order that we have. ie, "Rank the remaining operations by fewest required input members, then verified simple method" doesn't match the actual order that I think is has output, verified simple, empty request, ect....
|
|
||
| return BY_HAS_OUTPUT_FIRST | ||
| .thenComparing(byVerifiedSimpleFirst) | ||
| .thenComparing(BY_EMPTY_REQUEST_FIRST) |
There was a problem hiding this comment.
Should we also add a sort that prefers operations that have auth (ie, we don't prefer an option with noAuth since it wouldn't help us warm up signing related code)?
There was a problem hiding this comment.
Nice point 👍 Added now.
| * tie-break. | ||
| */ | ||
| private static Comparator<OperationModel> warmUpPreference(List<String> verifiedSimpleMethods) { | ||
| Comparator<OperationModel> byVerifiedSimpleFirst = |
There was a problem hiding this comment.
Is there a reason this isn't a constant like the others (eg, BY_EMPTY_REQUEST_FIRST)?
| private static final List<String> PREFERRED_VERBS = Arrays.asList("List", "Describe", "Get"); | ||
|
|
||
| private static final Comparator<OperationModel> BY_EMPTY_REQUEST_FIRST = | ||
| Comparator.comparing(op -> !acceptsEmptyRequest(op)); |
There was a problem hiding this comment.
This is just a personal readability preference - feel free to ignore - but I have a slightly hard team reading both the ! and then translating the true/false to compare order - what about:
Comparator.comparing(op -> acceptsEmptyRequest(op) ? 0 : 1);
| * call as a warm-up. | ||
| */ | ||
| public static Optional<OperationModel> selectWarmUpOperation(IntermediateModel model) { | ||
| List<String> verifiedSimpleMethods = model.getCustomizationConfig().getVerifiedSimpleMethods(); |
There was a problem hiding this comment.
I'm not sure if this is ever possible or not, but do we need to handle null here?
There was a problem hiding this comment.
No its not possible to be null here because its initialized as empty list
Updated the description , added auth too. |
2 participants
Motivation and Context
CRaC auto-priming needs the generated
ServiceWarmUpProvider.warmUp()to invoke one API operation with a synthetic request so the marshal/sign/unmarshal path is JIT-compiled before checkpoint. Not every operation is safe or useful for this call. This PR adds the codegen-time logic that picks the right operation per service. Wiring the selected operation into the generatedwarmUp()body comes in a follow-up PR.Modifications
Added
WarmUpOperationSelector, a codegen utility that selects one operation per service in two steps:Filter out operations that cannot be called with an all-null request. Removed: streaming or event-stream operations (they need extra arguments and special response handling), and deprecated operations.
Rank the remaining operations by: returns output (so the unmarshaller is primed too), is authenticated (so signing is primed too), verified simple method, accepts an empty request, fewest required input members, read-only verb (List over Describe over Get), then operation name for a deterministic tie-break.
Returns
Optional.empty()when no operation survives the filter; the consumer will generate a no-op provider in that case.Testing
License