Default endpointDiscovery to true for services that require it #1845
Conversation
| if (o.isEndpointOperation()) { | ||
| endpointOperation = o; | ||
| break; | ||
| } |
There was a problem hiding this comment.
We just use the last endpoint operation? Is that the right behavior?
There was a problem hiding this comment.
I'll admit this is not how I would have written it if I had done it myself, but it is actually a copy/paste from the v1 SDK. The logic is correct because it only needs to know there is at least one endpoint operation (and the last one suffices for that purpose), and the flag as to whether an endpoint is required is cumulative (once it gets set it doesn't get unset) so ends up effectively meaning 'at least one endpoint is required'.
There was a problem hiding this comment.
I added a couple of tests to assert this logic was working correctly as well.
There was a problem hiding this comment.
Can we clarify it a bit and make it a boolean or something?
There was a problem hiding this comment.
Changing it will require more refactoring than the change is worth in my opinion. At least it's in sync with v1 in this state. I'll add comments to explain what it's doing.
| } | ||
|
|
||
| if (o.getEndpointDiscovery() != null && o.getEndpointDiscovery().isRequired()) { | ||
| endpointCacheRequired = true; |
There was a problem hiding this comment.
So the "new" behavior is: if any operation requires endpoint discovery, enable it by default?
There was a problem hiding this comment.
Yes, exactly. I also stated that in the PR description.
| .addStatement("return this") | ||
| .build(); | ||
| } | ||
|
|
||
| private MethodSpec enableEndpointDiscovery() { | ||
| return MethodSpec.methodBuilder("enableEndpointDiscovery") |
There was a problem hiding this comment.
Deprecate here as well? Can we also remove it for new services?
There was a problem hiding this comment.
I will deprecate here as well. Do you have a strategy for removing it for new services? How do we identify 'new services'? One strategy is just to say 'service != DDB' which seems yuck.
There was a problem hiding this comment.
We could also add a customization: "enableEndpointDiscoveryMethodRequired" and just enable it for DynamoDB.
bmaizels
force-pushed
the
bmaizels/enable-default-endpoint-discovery
branch
from
71f07e1 to
379e27c
Compare
|
SonarCloud Quality Gate failed.
|
…babf0fa5a Pull request: release <- staging/3ce68cf4-4558-4ffd-a788-6f4babf0fa5a
This behavior should now match the v1 SDK.
License