[SERVER-55449] Add a tassertStatusOK() macro to assert_util.h Created: 23/Mar/21  Updated: 06/Dec/22  Resolved: 26/Mar/21

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Minor - P4
Reporter: Timour Katchaounov Assignee: Backlog - Service Architecture
Resolution: Duplicate Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-56831 Complete TODO listed in SERVER-55449 Closed
is related to SERVER-53380 Improve assert_util classes and imple... Backlog
Assigned Teams:
Service Arch
Participants:

 Description   

A common code pattern is to ensure that the result of some operation was Status::OK().

Example:

Instead of this:
 
tassert(3401202, "Error message", status);
 
do this:
 
tassertStatusOK(status);



 Comments   
Comment by Githook User [ 12/May/21 ]

Author:

{'name': 'Amirsaman Memaripour', 'email': 'amirsaman.memaripour@mongodb.com', 'username': 'samanca'}

Message: SERVER-56831 Complete TODO listed in SERVER-55449
Branch: master
https://github.com/mongodb/mongo/commit/3acbd9d5cd8fecdb00999eeb31be865ba0ca3244

Comment by Billy Donahue [ 26/Mar/21 ]

Maybe trying to give tassert/iassert a return value similar to that of uassertStatusOK(StatusWith<T>) should be considered as part of SERVER-53380.

Comment by Billy Donahue [ 26/Mar/21 ]

I think we'll just close it. The work to give uniform XassertX API support is really SERVER-53380.
I'll just mark this related.

Comment by David Storch [ 26/Mar/21 ]

Oh! Whoops! Yeah, the original request was not about extracting a T from a StatusWith<T>. I did not realize we already supported tassert(status), which is the same as tassertStatusOK() but by a different name. In that case I think this can just be closed, unless you want to convert the ticket into the auto val = tassert(sw); request.

Comment by Billy Donahue [ 26/Mar/21 ]

david.storch, would the proposed tassertStatusOK(status) be different from the existing tassert(status) ?

Maybe the extraction of value from a StatusWith<T> in a auto val = tassert(sw); would be nice. But it wasn't explicitly mentioned in the ticket I don't think, so I want to make sure I understand exactly what the request is.

That is, I think we can already do what's asked for, it just doesn't have the -StatusOK suffix on its name.

Thx.

Comment by David Storch [ 26/Mar/21 ]

Ah, good points Billy. I think the feature request is slightly misstated in the description. It would be about adding a tassertStatusOK() that works exactly like invariantStatusOK(). It would take just a Status, not a Status, code, and reason. I'll clarify the description. I do not believe the intent was to propose a different API for tassert() compared to the other assertion macros.

I will add this to the triage queue for the Service Arch team.

Comment by Billy Donahue [ 24/Mar/21 ]

Yes. I mean we should discuss it in a triage meeting as a feature request.
I'd have a few things to consider first, maybe.

You can already tassert(Status) or iassert(Status) directly.

Why does tassertStatusOK take an error code and a reason when the status already has these attributes?

Is `tassertStatusOK` different from `invariantStatusOK`, `uassertStatusOK`, or `massertStatusOK`?
None of those take a code and reason. They just take Status.

If it needs a msgid, then it's more analogous to fassertFailedWithStatus(msgid,status), but tassert doesn't otherwise follow fassert's API so maybe that would need to change.

We'll try to make it so that all the XassertX macros should have the same API if possible. Maybe even the same shared implementation (SERVER-53380). So if tassertStatusOK takes a code and reason, then ideally so would all the other XStatusOK macros.

So this ticket is one of:

  • closeable because tassert(status) already does the job
  • much bigger in scope so that all the XassertX macros accept code,reason parameters just like tassert does.
  • slightly bigger in scope because tassert is fundamentally different and needs a code,reason where other Xassert don't, and we need to document why.

The big picture is that we should be trying to make all the asserts have consistent feature set and calling convention. Right now they're very difficult to reason about, and the proposed tassert(msgid,msg,status) looks like it would be yet another syntax unique to tassert, which would make the XassertX API consistency problem a little worse.

Comment by David Storch [ 24/Mar/21 ]

billy.donahue is this something that the Service Arch team would consider picking up?

Generated at Thu Feb 08 05:36:30 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.