Skip to content

CDRIVER-1193, CDRIVER-1198: Expose mongoc_server_description_type_t and improve SDAM logging #322

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

CDRIVER-1193, CDRIVER-1198: Expose mongoc_server_description_type_t and improve SDAM logging #322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66def6c
CDRIVER-1193: Expose mongoc_server_description_type_t enum
jmikola Apr 12, 2016
1d3dced
CDRIVER-1198: Use strings for server and topology types in SDAM trans…
jmikola Apr 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 7 additions & 13 deletions doc/mongoc_server_description_type.page
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

<section id="synopsis">
<title>Synopsis</title>
<synopsis><code mime="text/x-csrc"><![CDATA[const char *
<synopsis><code mime="text/x-csrc"><![CDATA[mongoc_server_description_type_t
mongoc_server_description_type (const mongoc_server_description_t *description);

]]></code></synopsis>
Expand All @@ -27,18 +27,12 @@ mongoc_server_description_type (const mongoc_server_description_t *description);

<section id="description">
<title>Description</title>
<p>This function returns a string, one of the server types defined Server Discovery And Monitoring Spec:</p>
<list>
<item><p>Standalone</p></item>
<item><p>Mongos</p></item>
<item><p>PossiblePrimary</p></item>
<item><p>RSPrimary</p></item>
<item><p>RSSecondary</p></item>
<item><p>RSArbiter</p></item>
<item><p>RSOther</p></item>
<item><p>RSGhost</p></item>
<item><p>Unknown</p></item>
</list>
<p>Returns the <code xref="mongoc_server_description_type_t">mongoc_server_description_type_t</code> for the server.</p>
</section>

<section id="return">
<title>Returns</title>
<p>Returns the server type.</p>
</section>

</page>
36 changes: 36 additions & 0 deletions doc/mongoc_server_description_type_t.page
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?xml version="1.0"?>

<page id="mongoc_server_description_type_t"
type="guide"
style="class"
xmlns="http://projectmallard.org/1.0/"
xmlns:api="http://projectmallard.org/experimental/api/"
xmlns:ui="http://projectmallard.org/experimental/ui/">
<info>
<link type="guide" xref="index#api-reference" />
</info>
<title>mongoc_server_description_type_t</title>
<subtitle>Server description types</subtitle>

<section id="synopsis">
<title>Synopsis</title>
<synopsis><code mime="text/x-csrc"><![CDATA[typedef enum
{
MONGOC_SERVER_UNKNOWN,
MONGOC_SERVER_STANDALONE,
MONGOC_SERVER_MONGOS,
MONGOC_SERVER_POSSIBLE_PRIMARY,
MONGOC_SERVER_RS_PRIMARY,
MONGOC_SERVER_RS_SECONDARY,
MONGOC_SERVER_RS_ARBITER,
MONGOC_SERVER_RS_OTHER,
MONGOC_SERVER_RS_GHOST,
} mongoc_server_description_type_t;]]></code></synopsis>
</section>

<section id="description">
<title>Description</title>
<p>This enum describes server types, which are defined in the Server Discovery And Monitoring Spec.</p>
</section>

</page>
20 changes: 7 additions & 13 deletions src/mongoc/mongoc-server-description-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,9 @@
/* represent a server or topology with no replica set config version */
#define MONGOC_NO_SET_VERSION -1

typedef enum
{
MONGOC_SERVER_UNKNOWN,
MONGOC_SERVER_STANDALONE,
MONGOC_SERVER_MONGOS,
MONGOC_SERVER_POSSIBLE_PRIMARY,
MONGOC_SERVER_RS_PRIMARY,
MONGOC_SERVER_RS_SECONDARY,
MONGOC_SERVER_RS_ARBITER,
MONGOC_SERVER_RS_OTHER,
MONGOC_SERVER_RS_GHOST,
MONGOC_SERVER_DESCRIPTION_TYPES,
} 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jmikola jmikola Apr 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....


struct _mongoc_server_description_t
{
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errrrrrrr. Private functions should start with _, but I guess no other function here does that :/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


void
mongoc_server_description_update_rtt (mongoc_server_description_t *server,
int64_t new_time);
Expand Down
25 changes: 22 additions & 3 deletions src/mongoc/mongoc-server-description.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,13 +299,33 @@ mongoc_server_description_round_trip_time (mongoc_server_description_t *descript
* Discovery And Monitoring Spec.
*
* Returns:
* A string.
* A mongoc_server_description_type_t, this server's type.
*
*--------------------------------------------------------------------------
*/

mongoc_server_description_type_t
mongoc_server_description_type (const mongoc_server_description_t *description)
{
return description->type;
}

/*
*--------------------------------------------------------------------------
*
* mongoc_server_description_type_string --
*
* Get this server's type, one of the types defined in the Server
* Discovery And Monitoring Spec, as a string.
*
* Returns:
* A string denoting this server's type.
*
*--------------------------------------------------------------------------
*/

const char *
mongoc_server_description_type (mongoc_server_description_t *description)
mongoc_server_description_type_string (const mongoc_server_description_t *description)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing this used anywhere?

{
switch (description->type) {
case MONGOC_SERVER_UNKNOWN:
Expand All @@ -326,7 +346,6 @@ mongoc_server_description_type (mongoc_server_description_t *description)
return "RSOther";
case MONGOC_SERVER_RS_GHOST:
return "RSGhost";
case MONGOC_SERVER_DESCRIPTION_TYPES:
default:
MONGOC_ERROR ("Invalid mongoc_server_description_t type\n");
return "Invalid";
Expand Down
18 changes: 16 additions & 2 deletions src/mongoc/mongoc-server-description.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,20 @@
#include "mongoc-read-prefs.h"
#include "mongoc-host-list.h"

typedef enum
{
MONGOC_SERVER_UNKNOWN,
MONGOC_SERVER_STANDALONE,
MONGOC_SERVER_MONGOS,
MONGOC_SERVER_POSSIBLE_PRIMARY,
MONGOC_SERVER_RS_PRIMARY,
MONGOC_SERVER_RS_SECONDARY,
MONGOC_SERVER_RS_ARBITER,
MONGOC_SERVER_RS_OTHER,
MONGOC_SERVER_RS_GHOST,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the MONGOC_SERVER_DESCRIPTION_TYPES definition is decoupled from the enum, you probably should add at least a comment here that that value needs to be updated if the enum is extended. I would probably rather have the MONGOC_SERVER_DESCRIPTION_TYPES here to prevent them from going out of sync.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to define MONGOC_SERVER_DESCRIPTION_TYPES in the public API since it's only used internally; however, I've added a comment here mentioning that the constant should be updated if this enum is ever extended.

/* Update MONGOC_SERVER_DESCRIPTION_TYPES when extending this enum */
} mongoc_server_description_type_t;

typedef struct _mongoc_server_description_t mongoc_server_description_t;

void
Expand All @@ -39,8 +53,8 @@ mongoc_server_description_host (mongoc_server_description_t *description);
int64_t
mongoc_server_description_round_trip_time (mongoc_server_description_t *description);

const char *
mongoc_server_description_type (mongoc_server_description_t *description);
mongoc_server_description_type_t
mongoc_server_description_type (const mongoc_server_description_t *description);

const bson_t *
mongoc_server_description_ismaster (mongoc_server_description_t *description);
Expand Down
44 changes: 42 additions & 2 deletions src/mongoc/mongoc-topology-description.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1367,6 +1367,46 @@ gSDAMTransitionTable[MONGOC_SERVER_DESCRIPTION_TYPES][MONGOC_TOPOLOGY_DESCRIPTIO
}
};

/*
*--------------------------------------------------------------------------
*
* _mongoc_topology_description_type_string --
*
* Get this topology's type, one of the types defined in the Server
* Discovery And Monitoring Spec, as a string.
*
* NOTE: this method should only be called while holding the mutex on
* the owning topology object.
*
* Returns:
* A string.
*
* Side effects:
* None.
*
*--------------------------------------------------------------------------
*/
static const char *
_mongoc_topology_description_type_string (mongoc_topology_description_t *topology)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CDRIVER-1193 originally requested a mongoc_topology_description_type() getter, but I don't believe we need that in , which I don't believe we need that in PHPC or HHVM (I found no record of us accessing topology.type). In that case, this is just useful for the logging below and we can keep it private.

See also: https://github.com/mongodb/mongo-c-driver/pull/322/files#r59312484.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was something that @ajdavis mentioned in the email thread, and that's why I added it. I also don't think we use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only usecase for this function and mongoc_server_description_type_string () is the one TRACE() entry?
I don't think thats useful enough, and prefer keeping the trace lines as-is I guess

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only for CDRIVER-1198. As I mentioned below, the current trace lines require hoop-jumping to decipher, and a single log often has a bunch of those state changes. Using strings would be a huge convenience for debugging.

{
switch (topology->type) {
case MONGOC_TOPOLOGY_UNKNOWN:
return "Unknown";
case MONGOC_TOPOLOGY_SHARDED:
return "Sharded";
case MONGOC_TOPOLOGY_RS_NO_PRIMARY:
return "RSNoPrimary";
case MONGOC_TOPOLOGY_RS_WITH_PRIMARY:
return "RSWithPrimary";
case MONGOC_TOPOLOGY_SINGLE:
return "Single";
case MONGOC_TOPOLOGY_DESCRIPTION_TYPES:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modeled this after the original mongoc_server_description_type() implementation, which included this case. That said, it seems unnecessary since we have default: right below. Should I remove this?

default:
MONGOC_ERROR ("Invalid mongoc_topology_description_type_t type\n");
return "Invalid";
}
}

/*
*--------------------------------------------------------------------------
*
Expand Down Expand Up @@ -1403,9 +1443,9 @@ mongoc_topology_description_handle_ismaster (
error);

if (gSDAMTransitionTable[sd->type][topology->type]) {
TRACE("Transitioning to %d for %d", topology->type, sd->type);
TRACE("Transitioning to %s for %s", _mongoc_topology_description_type_string(topology), mongoc_server_description_type_string(sd));
gSDAMTransitionTable[sd->type][topology->type] (topology, sd);
} else {
TRACE("No transition entry to %d for %d", topology->type, sd->type);
TRACE("No transition entry to %s for %s", _mongoc_topology_description_type_string(topology), mongoc_server_description_type_string(sd));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if we'd like some line-wrapping here. If so, would that look something like the following?

TRACE(
   "No transition entry to %s for %s",
   _mongoc_topology_description_type_string(topology),
   mongoc_server_description_type_string(sd)
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems to be the only reason to introduce the _string () variants of these functions... I don't think thats good enough reason.
It'll cause useless extra work and headscratching in the future

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add some assertions before dereferencing that jump table.

Copy link
Member Author

@jmikola jmikola Apr 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjori: This was the only use of the private mongoc_server_description_type_string() function. I created the separate ticket to track the enhancement of using string names here. Since these messages frequently appear in driver logs, it'd be very helpful if we used descriptive names instead of integers, which require counting through enum values to resolve.

Maybe also add some assertions before dereferencing that jump table.

@chergert: I assume that applies to the if (gSDAMTransitionTable[sd->type][topology->type]) line?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmikola correct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened CDRIVER-1206 to track that suggestion.

}
}
26 changes: 13 additions & 13 deletions tests/test-mongoc-client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1349,7 +1349,7 @@ _test_mongoc_client_select_server (bool pooled)
mongoc_client_t *client;
mongoc_client_pool_t *pool = NULL;
mongoc_server_description_t *sd;
const char *server_type;
mongoc_server_description_type_t server_type;
bson_error_t error;
mongoc_read_prefs_t *prefs;

Expand All @@ -1367,9 +1367,9 @@ _test_mongoc_client_select_server (bool pooled)

ASSERT (sd);
server_type = mongoc_server_description_type (sd);
ASSERT (!strcmp (server_type, "Standalone") ||
!strcmp (server_type, "RSPrimary") ||
!strcmp (server_type, "Mongos"));
ASSERT (server_type != MONGOC_SERVER_STANDALONE ||
server_type != MONGOC_SERVER_RS_PRIMARY ||
server_type != MONGOC_SERVER_MONGOS);

mongoc_server_description_destroy (sd);
sd = mongoc_client_select_server (client,
Expand All @@ -1379,9 +1379,9 @@ _test_mongoc_client_select_server (bool pooled)

ASSERT (sd);
server_type = mongoc_server_description_type (sd);
ASSERT (!strcmp (server_type, "Standalone") ||
!strcmp (server_type, "RSPrimary") ||
!strcmp (server_type, "Mongos"));
ASSERT (server_type != MONGOC_SERVER_STANDALONE ||
server_type != MONGOC_SERVER_RS_PRIMARY ||
server_type != MONGOC_SERVER_MONGOS);

mongoc_server_description_destroy (sd);
prefs = mongoc_read_prefs_new (MONGOC_READ_SECONDARY);
Expand All @@ -1392,9 +1392,9 @@ _test_mongoc_client_select_server (bool pooled)

ASSERT (sd);
server_type = mongoc_server_description_type (sd);
ASSERT (!strcmp (server_type, "Standalone") ||
!strcmp (server_type, "RSSecondary") ||
!strcmp (server_type, "Mongos"));
ASSERT (server_type != MONGOC_SERVER_STANDALONE ||
server_type != MONGOC_SERVER_RS_SECONDARY ||
server_type != MONGOC_SERVER_MONGOS);

mongoc_server_description_destroy (sd);
mongoc_read_prefs_destroy (prefs);
Expand Down Expand Up @@ -1431,7 +1431,7 @@ _test_mongoc_client_select_server_error (bool pooled)
mongoc_server_description_t *sd;
bson_error_t error;
mongoc_read_prefs_t *prefs;
const char *server_type;
mongoc_server_description_type_t server_type;

if (pooled) {
uri = test_framework_get_uri ();
Expand Down Expand Up @@ -1467,8 +1467,8 @@ _test_mongoc_client_select_server_error (bool pooled)
if (client->topology->description.type == MONGOC_TOPOLOGY_SINGLE) {
ASSERT (sd);
server_type = mongoc_server_description_type (sd);
ASSERT (!strcmp (server_type, "Standalone") ||
!strcmp (server_type, "Mongos"));
ASSERT (server_type != MONGOC_SERVER_STANDALONE ||
server_type != MONGOC_SERVER_MONGOS);
mongoc_server_description_destroy (sd);
} else {
ASSERT (!sd);
Expand Down