You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/06/28 02:49:39 UTC
[GitHub] [pulsar] RobertIndie opened a new pull request, #16256: [fix][client] Specify the init parameters value in the OpSendMsg
RobertIndie opened a new pull request, #16256:
URL: https://github.com/apache/pulsar/pull/16256
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
### Motivation
Some parameters in OpSendMsg are not specified with initialized values.
### Modifications
* Specify the init value for these parameters according to the value in the OpSendMsg.recyle().
### 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)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #16256: [fix][client] Specify the init parameters value in the OpSendMsg
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on PR #16256:
URL: https://github.com/apache/pulsar/pull/16256#issuecomment-1168196968
> Please explain what's the affect of these fields without initial values.
I have explained it in the PR description. Is there something unclear?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #16256: [fix][client] Specify the init parameters value in the OpSendMsg
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on PR #16256:
URL: https://github.com/apache/pulsar/pull/16256#issuecomment-1168201771
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie merged pull request #16256: [fix][client] Add initialization for the OpSendMsg
Posted by GitBox <gi...@apache.org>.
RobertIndie merged PR #16256:
URL: https://github.com/apache/pulsar/pull/16256
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16256: [fix][client] Specify the init parameters value in the OpSendMsg
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16256:
URL: https://github.com/apache/pulsar/pull/16256#issuecomment-1168198787
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16256: [fix][client] Specify the init parameters value in the OpSendMsg
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16256:
URL: https://github.com/apache/pulsar/pull/16256#issuecomment-1168202259
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] HQebupt commented on a diff in pull request #16256: [fix][client] Specify the init parameters value in the OpSendMsg
Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #16256:
URL: https://github.com/apache/pulsar/pull/16256#discussion_r907996118
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -1337,15 +1337,15 @@ protected static final class OpSendMsg {
SendCallback callback;
Runnable rePopulate;
ChunkedMessageCtx chunkedMessageCtx;
- long uncompressedSize;
- long sequenceId;
- long createdAt;
- long firstSentAt;
- long lastSentAt;
- int retryCount;
Review Comment:
Does it have any effect if no explicit initial value ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16256: [fix][client] Specify the init parameters value in the OpSendMsg
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16256:
URL: https://github.com/apache/pulsar/pull/16256#issuecomment-1168201122
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:
```java
private void initialize() {
x = 100;
}
public static RecyclableInteger create() {
final RecyclableInteger i = RECYCLER.get();
i.initialize();
return i;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16256: [fix][client] Specify the init parameters value in the OpSendMsg
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16256:
URL: https://github.com/apache/pulsar/pull/16256#issuecomment-1168198165
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,
```java
@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);
}
}
```
```java
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a diff in pull request #16256: [fix][client] Specify the init parameters value in the OpSendMsg
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on code in PR #16256:
URL: https://github.com/apache/pulsar/pull/16256#discussion_r908009895
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java:
##########
@@ -1337,15 +1337,15 @@ protected static final class OpSendMsg {
SendCallback callback;
Runnable rePopulate;
ChunkedMessageCtx chunkedMessageCtx;
- long uncompressedSize;
- long sequenceId;
- long createdAt;
- long firstSentAt;
- long lastSentAt;
- int retryCount;
Review Comment:
Yes. These initial values are all relied upon by other codes. Such as here:
https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L1409-L1422
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org