Skip to content

Remove dead code in ReplyHeader #1231

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

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Remove dead code in ReplyHeader #1231

merged 1 commit into from
Nov 6, 2023

Conversation

jyemin
Copy link
Collaborator

@jyemin jyemin commented Oct 24, 2023

The dead code is a remnant of when the driver supported the full range of OP_REPLY wire protocol message usage. Since the driver now only runs against MongoDB releases that support OP_MSG, OP_REPLY is only used for the response to the hello command in the handshake, and therefore most of the OP_REPLY-handling code is no longer on any execution paths.

JAVA-5204

The dead code is a remnant of when the driver supported the
full range of OP_REPLY wire protocol message usage.  Since the driver
now only runs against MongoDB releases that support OP_MSG,
OP_REPLY is only used for the response to the `hello` command
in the handshake, and therefore most of the OP_REPLY-handling code is
no longer on any execution paths.

JAVA-5204
@jyemin jyemin self-assigned this Oct 24, 2023
opMsgFlagBits = header.getInt();
header.get(); // ignore payload type
int flagBits = header.getInt();
hasMoreToCome = (flagBits & (1 << 1)) != 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to store all the flag bigs since we only ever examine this one.

}
}

ReplyMessage(final ReplyHeader replyHeader, final long requestId) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This constructor was only used by a test, so removed it and refactored the test to use the remaining constructor.

@@ -35,50 +32,24 @@
*/
public class ReplyMessage<T> {

private final ReplyHeader replyHeader;
private final List<T> documents;
private final T document;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In practice this is always a list of exactly 1, so get rid of the list

@@ -63,7 +59,7 @@ class CommandMessageSpecification extends Specification {
.serverType(serverType as ServerType)
.sessionSupported(true)
.build(),
responseExpected, exhaustAllowed, null, null, clusterConnectionMode, null)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to test this flag bit since the driver never uses it.

}

@Test(expected = MongoInternalException.class)
public void shouldThrowExceptionIfOpCodeIsIncorrect() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is actually a test of ReplyHeader, not ReplyMessage, and it duplicates an existing test in ReplyHeaderSpecification. That's why it's been deleted. (Migrating to use assertThrows is what clued me in to that).

@jyemin jyemin requested review from stIncMale and vbabanin October 24, 2023 18:30
Copy link
Member

@vbabanin vbabanin left a comment

Choose a reason for hiding this comment

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

LGTM! I've noticed that OP_REPLY is unsupported as of MongoDB 5.1. Does this mean that driver 5.0 will not be compatible with MongoDB servers prior to 5.1?

@jyemin
Copy link
Collaborator Author

jyemin commented Nov 6, 2023

No, nothing like that. The driver (and this code) fully supports OP_MSG, which is supported by all server versions >= 3.6. OP_REPLY is only used in very limited circumstances by the driver: when sending the first handshake message on a connection, when we don't know what server version we're connected to. So the driver sends an "ismaster" command using OP_QUERY, and the server responds with an OP_REPLY message. The driver will then switch to OP_MSG for all subsequent messages.

@jyemin jyemin merged commit 1860cbf into mongodb:5.x Nov 6, 2023
@jyemin jyemin deleted the j5204 branch November 6, 2023 15:09
jyemin added a commit that referenced this pull request Dec 4, 2023
The dead code is a remnant of when the driver supported the
full range of OP_REPLY wire protocol message usage.  Since the driver
now only runs against MongoDB releases that support OP_MSG,
OP_REPLY is only used for the response to the `hello` command
in the handshake, and therefore most of the OP_REPLY-handling code is
no longer on any execution paths.

JAVA-5204
jyemin added a commit that referenced this pull request Dec 5, 2023
The dead code is a remnant of when the driver supported the
full range of OP_REPLY wire protocol message usage.  Since the driver
now only runs against MongoDB releases that support OP_MSG,
OP_REPLY is only used for the response to the `hello` command
in the handshake, and therefore most of the OP_REPLY-handling code is
no longer on any execution paths.

JAVA-5204
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants