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/09/09 04:34:49 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request, #17559: [fix][broker] Fix potential can't remove producer/reader reference
Technoboy- opened a new pull request, #17559:
URL: https://github.com/apache/pulsar/pull/17559
### Motivation
If the `producer`/`reader` close with the exception, there may exist references that can't remove from the list.
### Documentation
- [x] `doc-not-needed`
(Please explain why)
--
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] Technoboy- merged pull request #17559: [fix][broker] Fix potential can't remove producer/reader reference
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #17559:
URL: https://github.com/apache/pulsar/pull/17559
--
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] lhotari commented on a diff in pull request #17559: [fix][broker] Fix potential can't remove producer/reader reference
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17559:
URL: https://github.com/apache/pulsar/pull/17559#discussion_r966643646
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/TopicPoliciesSystemTopicClient.java:
##########
@@ -121,15 +121,17 @@ private String getEventKey(PulsarEvent event) {
@Override
public void close() throws IOException {
- this.producer.close();
- systemTopicClient.getWriters().remove(TopicPolicyWriter.this);
+ try {
+ closeAsync().get();
+ } catch (Exception e) {
+ throw new IOException(e);
Review Comment:
this seems wrong. I'd suggest something similar as what is used in BrokerService.close: https://github.com/apache/pulsar/blob/a7f1a5657782ff7ac91a89afdd49568d74d111f5/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L713-L721
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/TopicPoliciesSystemTopicClient.java:
##########
@@ -184,15 +186,17 @@ public CompletableFuture<Boolean> hasMoreEventsAsync() {
@Override
public void close() throws IOException {
- this.reader.close();
- systemTopic.getReaders().remove(TopicPolicyReader.this);
+ try {
+ closeAsync().get();
+ } catch (Exception e) {
+ throw new IOException(e);
+ }
Review Comment:
this seems wrong. I'd suggest something similar as what is used in BrokerService.close: https://github.com/apache/pulsar/blob/a7f1a5657782ff7ac91a89afdd49568d74d111f5/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L713-L721
--
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] Technoboy- commented on a diff in pull request #17559: [fix][broker] Fix potential can't remove producer/reader reference
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #17559:
URL: https://github.com/apache/pulsar/pull/17559#discussion_r966768893
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/TopicPoliciesSystemTopicClient.java:
##########
@@ -121,15 +121,17 @@ private String getEventKey(PulsarEvent event) {
@Override
public void close() throws IOException {
- this.producer.close();
- systemTopicClient.getWriters().remove(TopicPolicyWriter.this);
+ try {
+ closeAsync().get();
+ } catch (Exception e) {
+ throw new IOException(e);
Review Comment:
fixed.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/TopicPoliciesSystemTopicClient.java:
##########
@@ -184,15 +186,17 @@ public CompletableFuture<Boolean> hasMoreEventsAsync() {
@Override
public void close() throws IOException {
- this.reader.close();
- systemTopic.getReaders().remove(TopicPolicyReader.this);
+ try {
+ closeAsync().get();
+ } catch (Exception e) {
+ throw new IOException(e);
+ }
Review Comment:
fixed
--
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] mattisonchao commented on a diff in pull request #17559: [fix][broker] Fix potential can't remove producer/reader reference
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #17559:
URL: https://github.com/apache/pulsar/pull/17559#discussion_r966677398
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/TopicPoliciesSystemTopicClient.java:
##########
@@ -121,15 +121,17 @@ private String getEventKey(PulsarEvent event) {
@Override
public void close() throws IOException {
- this.producer.close();
- systemTopicClient.getWriters().remove(TopicPolicyWriter.this);
+ try {
+ closeAsync().get();
+ } catch (Exception e) {
+ throw new IOException(e);
Review Comment:
+1
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/TopicPoliciesSystemTopicClient.java:
##########
@@ -184,15 +186,17 @@ public CompletableFuture<Boolean> hasMoreEventsAsync() {
@Override
public void close() throws IOException {
- this.reader.close();
- systemTopic.getReaders().remove(TopicPolicyReader.this);
+ try {
+ closeAsync().get();
+ } catch (Exception e) {
+ throw new IOException(e);
+ }
Review Comment:
+1
--
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 #17559: [fix][broker] Fix potential can't remove producer/reader reference
Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #17559:
URL: https://github.com/apache/pulsar/pull/17559#discussion_r968413682
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/TopicPoliciesSystemTopicClient.java:
##########
@@ -184,15 +194,23 @@ public CompletableFuture<Boolean> hasMoreEventsAsync() {
@Override
public void close() throws IOException {
- this.reader.close();
- systemTopic.getReaders().remove(TopicPolicyReader.this);
+ try {
+ closeAsync().get();
+ } catch (ExecutionException e) {
+ if (e.getCause() instanceof IOException) {
+ throw (IOException) e.getCause();
+ } else {
+ throw new PulsarServerException(e.getCause());
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
}
@Override
public CompletableFuture<Void> closeAsync() {
- return reader.closeAsync().thenCompose(v -> {
Review Comment:
This line is key point. The previous `thenCompose` is replaced by `thenApply` just to save `CompletableFuture`. Right?
--
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] Technoboy- closed pull request #17559: [fix][broker] Fix potential can't remove producer/reader reference
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #17559: [fix][broker] Fix potential can't remove producer/reader reference
URL: https://github.com/apache/pulsar/pull/17559
--
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] Technoboy- commented on a diff in pull request #17559: [fix][broker] Fix potential can't remove producer/reader reference
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #17559:
URL: https://github.com/apache/pulsar/pull/17559#discussion_r969090593
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/TopicPoliciesSystemTopicClient.java:
##########
@@ -184,15 +194,23 @@ public CompletableFuture<Boolean> hasMoreEventsAsync() {
@Override
public void close() throws IOException {
- this.reader.close();
- systemTopic.getReaders().remove(TopicPolicyReader.this);
+ try {
+ closeAsync().get();
+ } catch (ExecutionException e) {
+ if (e.getCause() instanceof IOException) {
+ throw (IOException) e.getCause();
+ } else {
+ throw new PulsarServerException(e.getCause());
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
}
@Override
public CompletableFuture<Void> closeAsync() {
- return reader.closeAsync().thenCompose(v -> {
Review Comment:
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