Skip to content
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

[WEJBHTTP-139] Final round of cleanup refactorings accross all modules #259

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

ropalka
Copy link
Collaborator

@ropalka ropalka commented Dec 11, 2024

@ropalka ropalka requested a review from fl4via as a code owner December 11, 2024 11:05
@ropalka ropalka requested review from rachmatowicz and removed request for fl4via December 11, 2024 11:06
Copy link
Collaborator

@rachmatowicz rachmatowicz left a comment

Choose a reason for hiding this comment

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

Generally, looks fine although there is some disagreement about removing context from variable names :-)

@@ -42,6 +42,9 @@ final class Constants {
// context path
static final String TXN_CONTEXT = "/txn";

// params
static final String OPC_QUERY_PARAMETER = "opc";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This string literal doesn't say much to me. A query parameter called "onePhase" would be more descriptive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can change that in future versions of the protocol. We have to keep that name now for backward compatibility.

@@ -73,4 +77,38 @@ public void write(final byte[] b, final int off, final int len) throws IOExcepti

}

private static final class UnflushableByteOutput implements ByteOutput {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Motivation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is the way this protocol was implemented. Output is always sent at once to prevent fragmentation on the wire.
I maintained it for backward compatibility. For future version of the protocol we could revisit it if its really necessary.

…legalArgumentException if HeadersHelper method parameter is null
… 'output' variable names for ObjectInput and ObjectOutput
…t and long as method parameters in HeadersHelper
…tead of txn for TransactionInfo parameter names
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.

2 participants