-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed diagnostics information and other APIs on cosmos stored procedure response #16946
Fixed diagnostics information and other APIs on cosmos stored procedure response #16946
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than the following suggestion LGTM.
with this change essentially similar to CosmosItemResponse we are implementing all these APIs here instead of relying on the parent class.
So I would like to suggest that we drop the inheritance on the CosmosResponse for this class.
I know that theoretically speaking it can be considered a breaking change but in practice it is not breaking change as you are implementing all the required methods.
@@ -90,13 +94,56 @@ public double getRequestCharge() { | |||
return super.getRequestCharge(); | |||
} | |||
|
|||
@Override | |||
public String getMaxResourceQuota() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with this change essentially similar to CosmosItemResponse
we are implementing all these APIs here instead of relying on the parent class.
So I would like to suggest that we drop the inheritance on the CosmosResponse
for this class.
I know that theoretically speaking it can be considered a breaking change but in practice it is not breaking change as you are implementing all the required methods.
...cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosStoredProcedureResponse.java
Show resolved
Hide resolved
@@ -81,7 +81,7 @@ public void conflictResolutionPolicyCRUD() { | |||
// when (e.StatusCode == HttpStatusCode.BadRequest) | |||
CosmosException dce = Utils.as(e, CosmosException.class); | |||
if (dce != null && dce.getStatusCode() == 400) { | |||
assertThat(dce.getMessage()).contains("Invalid path '\\/a\\/b' for last writer wins conflict resolution"); | |||
assertThat(dce.getMessage()).contains("Invalid path '\\\\\\/a\\\\\\/b' for last writer wins conflict resolution"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this because of service side message change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the reason :)
/azp run java cosmos tests |
No pipelines are associated with this pull request. |
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for fixing the multimaster conflict test 👍
/check-enforcer override |
cosmos diagnostics
and other APIs onCosmosStoredProcedureResponse