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] Add initialization for the OpSendMsg #16256

Merged
merged 2 commits into from
Jul 8, 2022

Conversation

RobertIndie
Copy link
Member

@RobertIndie RobertIndie commented Jun 28, 2022

Motivation

Some parameters in OpSendMsg are not specified with initialized values.

These initial values are all relied upon by other codes. Such as here:

//firstSentAt and lastSentAt maybe -1, it means that the message didn't flush to channel.
String errMsg = String.format(
"%s : createdAt %s seconds ago, firstSentAt %s seconds ago, lastSentAt %s seconds ago, "
+ "retryCount %s",
te.getMessage(),
RelativeTimeUtil.nsToSeconds(ns - this.createdAt),
RelativeTimeUtil.nsToSeconds(this.firstSentAt <= 0
? this.firstSentAt
: ns - this.firstSentAt),
RelativeTimeUtil.nsToSeconds(this.lastSentAt <= 0
? this.lastSentAt
: ns - this.lastSentAt),
retryCount
);

Modifications

  • Add the initialization for the OpSendMsg.

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@RobertIndie RobertIndie self-assigned this Jun 28, 2022
@RobertIndie RobertIndie added type/bug The PR fixed a bug or issue reported a bug area/client component/client-java labels Jun 28, 2022
@RobertIndie RobertIndie added this to the 2.11.0 milestone Jun 28, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 28, 2022
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Please explain what's the affect of these fields without initial values.

@RobertIndie
Copy link
Member Author

Please explain what's the affect of these fields without initial values.

I have explained it in the PR description. Is there something unclear?

@BewareMyPower
Copy link
Contributor

In addition, adding initial values to a recyclable class is dangerous. For a recyclable class, we should make constructors private and only expose some factory methods and set the value in these factory methods.

The reason is that if the object was reused from the pool, the initial value should be what was set in recycle() method.

For example,

@Data
class RecyclableInteger {
    private static final Recycler<RecyclableInteger> RECYCLER = new Recycler<RecyclableInteger>() {
        @Override
        protected RecyclableInteger newObject(Handle<RecyclableInteger> handle) {
            return new RecyclableInteger(handle);
        }
    };

    private final Recycler.Handle<RecyclableInteger> handle;
    private int x = 100; // initial value: 100

    private RecyclableInteger(Recycler.Handle<RecyclableInteger> handle) {
        this.handle = handle;
    }

    public static RecyclableInteger create() {
        return RECYCLER.get(); // retrieve an object without modifying the value of `x`
    }

    public void recycle() {
        this.x = 0; // recycled value: 0
        handle.recycle(this);
    }
}
        RecyclableInteger i = RecyclableInteger.create();
        System.out.println(i.getX()); // 100 as we expected
        i.recycle();
        i = RecyclableInteger.create();
        System.out.println(i.getX()); // 0, which is unexpected

@BewareMyPower
Copy link
Contributor

I have explained it in the PR description. Is there something unclear?

Sorry at the time I replied, the PR description was not updated. Then, please see my another comment.

@BewareMyPower
Copy link
Contributor

The best practice might be adding an initialize() method to a recyclable class and calling this method at the beginning of each factory method, like:

    private void initialize() {
        x = 100;
    }

    public static RecyclableInteger create() {
        final RecyclableInteger i = RECYCLER.get();
        i.initialize();
        return i;
    }

@RobertIndie
Copy link
Member Author

In addition, adding initial values to a recyclable class is dangerous. For a recyclable class, we should make constructors private and only expose some factory methods and set the value in these factory methods.

The reason is that if the object was reused from the pool, the initial value should be what was set in recycle() method.

Before this PR, there is already an inconsistent issue in the OpSendMsg. The value of the OpSendMsg just initialized is not the same as the OpSendMsg just being recycled. This PR is to fix this inconsistency. I was wondering if we could have the recyle method call some kind of initialization method so we don't need to maintain the same initialization value in two places.

@BewareMyPower
Copy link
Contributor

if we could have the recyle method call some kind of initialization method so we don't need to maintain the same initialization value in two places.

Yes.

@RobertIndie RobertIndie changed the title [fix][client] Specify the init parameters value in the OpSendMsg [fix][client] Add initialization for the OpSendMsg Jun 30, 2022
@RobertIndie
Copy link
Member Author

@BewareMyPower Hi, I have added the initialization for the OpSendMsg, PTAL again. Thanks.

@nodece nodece self-requested a review July 7, 2022 10:36
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

@RobertIndie RobertIndie merged commit e997472 into apache:master Jul 8, 2022
zymap pushed a commit to zymap/pulsar that referenced this pull request Jul 11, 2022
### Motivation

Some parameters in OpSendMsg are not specified with initialized values. 

These initial values are all relied upon by other codes. Such as here:
https://github.com/apache/pulsar/blob/eec46ddcba4d2b4f956e1b4d63154cc43087f507/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L1409-L1422

### Modifications

* Add the initialization for the OpSendMsg.
wuxuanqicn pushed a commit to wuxuanqicn/pulsar that referenced this pull request Jul 14, 2022
### Motivation

Some parameters in OpSendMsg are not specified with initialized values. 

These initial values are all relied upon by other codes. Such as here:
https://github.com/apache/pulsar/blob/eec46ddcba4d2b4f956e1b4d63154cc43087f507/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L1409-L1422

### Modifications

* Add the initialization for the OpSendMsg.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client doc-not-needed Your PR changes do not impact docs type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants