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

[fix][client]sendMessage param miss in createOpSendMsg cause CI Build Error #17297

Closed
wants to merge 2 commits into from

Conversation

Nicklee007
Copy link
Contributor

Motivation

Fix sendMessage param miss in createOpSendMsg which will cause build error.

Build error cause
https://github.com/apache/pulsar/runs/8032271109?check_suite_focus=true

Error:  COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
Error:  /Users/runner/work/pulsar/pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java:[224,39] no suitable method found for sendMessage(long,long,int,org.apache.pulsar.common.api.proto.MessageMetadata,io.netty.buffer.ByteBuf)
    method org.apache.pulsar.client.impl.ProducerImpl.sendMessage(long,long,int,org.apache.pulsar.client.api.MessageId,org.apache.pulsar.common.api.proto.MessageMetadata,io.netty.buffer.ByteBuf) is not applicable
      (actual and formal argument lists differ in length)
    method org.apache.pulsar.client.impl.ProducerImpl.sendMessage(long,long,long,int,org.apache.pulsar.common.api.proto.MessageMetadata,io.netty.buffer.ByteBuf) is not applicable
      (actual and formal argument lists differ in length)

public OpSendMsg createOpSendMsg() throws IOException {
if (messages.size() == 1) {
messageMetadata.clear();
messageMetadata.copyFrom(messages.get(0).getMessageBuilder());
ByteBuf encryptedPayload = producer.encryptMessage(messageMetadata, getCompressedBatchMetadataAndPayload());
ByteBufPair cmd = producer.sendMessage(producer.producerId, messageMetadata.getSequenceId(),
1, messageMetadata, encryptedPayload);

Modifications

  1. add the missed sendMessage param;

Documentation

  • doc-not-needed
    (Please explain why)

@Nicklee007 Nicklee007 changed the title [fix][client]Fix sendMessage param miss in createOpSendMsg [fix][client]sendMessage param miss in createOpSendMsg cause CI Build Error Aug 26, 2022
@gaozhangmin
Copy link
Contributor

Why the CI doesn't run failed for now?

@coderzc
Copy link
Member

coderzc commented Aug 26, 2022

Why the CI doesn't run failed for now?

I think because we don‘t mandatorily rebase master before merge PR.

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 26, 2022
@@ -220,7 +220,7 @@ public OpSendMsg createOpSendMsg() throws IOException {
messageMetadata.copyFrom(messages.get(0).getMessageBuilder());
ByteBuf encryptedPayload = producer.encryptMessage(messageMetadata, getCompressedBatchMetadataAndPayload());
ByteBufPair cmd = producer.sendMessage(producer.producerId, messageMetadata.getSequenceId(),
1, messageMetadata, encryptedPayload);
1, messages.get(0).getMessageId(), messageMetadata, encryptedPayload);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right. Are you sure that this is the correct way to fix this?

Copy link
Member

Choose a reason for hiding this comment

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

See #17195 . PIP-180 is for the shadow replicator. null should be passed here. I created another PR #17300 since I didn't notice this PR and I digged the reason why things broke.

Copy link
Member

Choose a reason for hiding this comment

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

@Jason918 PTAL. isn't it null that should be passed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lhotari Actually, both is fine. ShadowReplicator won't use batch producing.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

@lhotari is right

closing this PR, thank you @Nicklee007

@eolivelli eolivelli closed this Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants