CDRIVER-1193, CDRIVER-1198: Expose mongoc_server_description_type_t and improve SDAM logging #322
Conversation
| } mongoc_server_description_type_t; | ||
| /* this is used for declaring gSDAMTransitionTable and should be defined as the | ||
| * number of unique states in mongoc_server_description_type_t */ | ||
| #define MONGOC_SERVER_DESCRIPTION_TYPES 9 |
There was a problem hiding this comment.
If I understood to previous code correctly, defining this as the last enum value simply allowed you to keep track of the number of unique states for purposes of declaring the SDAM transition table. This value was never intended to be a type itself, which is its switch case was "invalid". I believe this #define accomplishes that and I left a comment here for good measure.
There was a problem hiding this comment.
It tells us how many elements we need to account for in the transition table and was a neat way of keeping track of it.
I think moving it to the .c file it is used in may be the best course of action
There was a problem hiding this comment.
mongoc-topology-description.c? That seems a bit odd given that it has a MONGOC_SERVER_DESCRIPTION prefix. If moved there, would you want it at the top of the C file below the #include lines or right above the jump table definition?
There was a problem hiding this comment.
It seems weird to define an enum in one place, the count of it in another place, and then its only used in the 3rd place.
Its unfortunate that this enum must be exposed, because it means we are knowingly breaking the ABI as soon as we add another entry to it, which will break your "enum to string" right off the bat.
I strongly recommend instead that we rather return the stringification of the types.
If you want to represent that as a number somewhere else, then thats fine. Any unknown strings can then be represented as a 0 in the app or something....
jmikola
force-pushed
the
cdriver-1193
branch
5 times, most recently
from
5a79de9 to
cbd3520
Compare
| @@ -104,6 +94,10 @@ mongoc_server_description_set_set_version (mongoc_server_description_t *descript | |||
| void | |||
| mongoc_server_description_set_election_id (mongoc_server_description_t *description, | |||
| const bson_oid_t *election_id); | |||
|
|
|||
| const char * | |||
| mongoc_server_description_type_string (const mongoc_server_description_t *description); | |||
There was a problem hiding this comment.
Errrrrrrr. Private functions should start with _, but I guess no other function here does that :/
There was a problem hiding this comment.
Existing private functions in this file didn't use an underscore prefix, so I didn't use it. I can change this in the second commit if you decide that CDRIVER-1198 is worthwhile.
This changes mongoc_server_description_type() to return the enum value instead of a string.
|
Closing this. CDRIVER-1198 was split off into #323 and we decided to maintain a server description type map manually in the PHP driver (see mongodb/mongo-php-driver#291). |
Cross-referencing with mongodb/mongo-php-driver#291.