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 2021/02/26 11:21:08 UTC
[GitHub] [pulsar] golden-yang opened a new pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
golden-yang opened a new pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- 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)**
-->
Fix #9633
### Motivation
When we set `functionsWorkerEnabled=false` in `broker.conf`, broker don't support management function of `functionWorker`.
But when we try to appy those method. Broker will throw NullPointerException instead of throwing a more user friendly error that explain the problem.
This PR is trying to fix it.
### Modifications
* Throw an exception instead of returning null
* Add unit test for normal and abnormal conditions
### Documentation
- Does this pull request introduce a new feature? ( no)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#discussion_r585686991
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -957,8 +957,9 @@ public NamespaceService getNamespaceService() {
return functionWorkerService;
}
- public WorkerService getWorkerService() {
- return functionWorkerService.orElse(null);
+ public WorkerService getWorkerService() throws UnsupportedOperationException {
+ return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Function Worker is not " +
+ "enabled, probably functionsWorkerEnabled is set to false"));
Review comment:
In my opinion, If throw exception while getWorkerService(), this might break some other places since it might just want to return a null value. As I said before if all the places are ok for handling the exception that getWorkerService() throws, I'm ok. I just worry about introducing some break changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#discussion_r583643179
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/PulsarServiceTest.java
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker;
+
+import static org.mockito.Mockito.mock;
+import java.util.Optional;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.functions.worker.WorkerConfig;
+import org.apache.pulsar.functions.worker.WorkerService;
+import org.testng.annotations.Test;
+
+
+@Slf4j
+public class PulsarServiceTest {
+
+ @Test
+ public void testGetWorkerService() {
+ ServiceConfiguration configuration = new ServiceConfiguration();
+ configuration.setZookeeperServers("localhost");
+ configuration.setClusterName("clusterName");
+ configuration.setFunctionsWorkerEnabled(true);
+ PulsarService pulsarService = new PulsarService(configuration, new WorkerConfig(),
+ Optional.of(mock(WorkerService.class)), (exitCode) -> {});
+
+ pulsarService.getWorkerService();
Review comment:
please add assertSame, otherwise you are not testing that the method is doing what it is expected to do
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/PulsarServiceTest.java
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker;
+
+import static org.mockito.Mockito.mock;
+import java.util.Optional;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.functions.worker.WorkerConfig;
+import org.apache.pulsar.functions.worker.WorkerService;
+import org.testng.annotations.Test;
+
+
+@Slf4j
+public class PulsarServiceTest {
+
+ @Test
+ public void testGetWorkerService() {
+ ServiceConfiguration configuration = new ServiceConfiguration();
+ configuration.setZookeeperServers("localhost");
+ configuration.setClusterName("clusterName");
+ configuration.setFunctionsWorkerEnabled(true);
+ PulsarService pulsarService = new PulsarService(configuration, new WorkerConfig(),
+ Optional.of(mock(WorkerService.class)), (exitCode) -> {});
+
+ pulsarService.getWorkerService();
+ }
+
+ /**
+ * Verifies that the getWorkerService throws {@link UnsupportedOperationException}
+ * when functionsWorkerEnabled is set to false .
+ */
+ @Test(expectedExceptions = UnsupportedOperationException.class)
+ public void testGetWorkerServiceException() throws Exception {
+ ServiceConfiguration configuration = new ServiceConfiguration();
+ configuration.setZookeeperServers("localhost");
+ configuration.setClusterName("clusterName");
+ configuration.setFunctionsWorkerEnabled(false);
+ PulsarService pulsarService = new PulsarService(configuration, new WorkerConfig(),
+ Optional.empty(), (exitCode) -> {});
+
+ pulsarService.getWorkerService();
Review comment:
can you also please test against the error message?
the main point of this issue is that the user is getting a bad experience.
If we ensure that the message is meaningful then we are sure that we are addressing the problem for the user
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -957,8 +957,9 @@ public NamespaceService getNamespaceService() {
return functionWorkerService;
}
- public WorkerService getWorkerService() {
- return functionWorkerService.orElse(null);
+ public WorkerService getWorkerService() throws UnsupportedOperationException {
+ return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Functions worker " +
Review comment:
what about:
"Pulsar Function Worker is not enabled, probably functionsWorkerEnabled is set to false"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] golden-yang commented on pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
golden-yang commented on pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#issuecomment-788834087
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#discussion_r586093541
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -957,8 +957,9 @@ public NamespaceService getNamespaceService() {
return functionWorkerService;
}
- public WorkerService getWorkerService() {
- return functionWorkerService.orElse(null);
+ public WorkerService getWorkerService() throws UnsupportedOperationException {
+ return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Function Worker is not " +
+ "enabled, probably functionsWorkerEnabled is set to false"));
Review comment:
Ok
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] golden-yang commented on a change in pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
golden-yang commented on a change in pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#discussion_r586115637
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -957,8 +957,9 @@ public NamespaceService getNamespaceService() {
return functionWorkerService;
}
- public WorkerService getWorkerService() {
- return functionWorkerService.orElse(null);
+ public WorkerService getWorkerService() throws UnsupportedOperationException {
+ return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Function Worker is not " +
+ "enabled, probably functionsWorkerEnabled is set to false"));
Review comment:
@codelipenghui Thank you for your suggestion. I understand your consideration.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#discussion_r585706716
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -957,8 +957,9 @@ public NamespaceService getNamespaceService() {
return functionWorkerService;
}
- public WorkerService getWorkerService() {
- return functionWorkerService.orElse(null);
+ public WorkerService getWorkerService() throws UnsupportedOperationException {
+ return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Function Worker is not " +
+ "enabled, probably functionsWorkerEnabled is set to false"));
Review comment:
@codelipenghui I share your opinion.
but if all of the tests are passing, especially integration tests, we should be safe.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] golden-yang commented on a change in pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
golden-yang commented on a change in pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#discussion_r584462661
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -957,8 +957,9 @@ public NamespaceService getNamespaceService() {
return functionWorkerService;
}
- public WorkerService getWorkerService() {
- return functionWorkerService.orElse(null);
+ public WorkerService getWorkerService() throws UnsupportedOperationException {
+ return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Function Worker is not " +
+ "enabled, probably functionsWorkerEnabled is set to false"));
Review comment:
Using getWorkerServiceOpt directly may be a more elegant implementation, but it will change too many places, and the expected behavior of most methods is also to throw a clearer exception.
In your opinion, which one is better?
Simplify the implementation or use a more elegant return value.
Maybe you have a better idea, Thank you again for your advice
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] golden-yang removed a comment on pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
golden-yang removed a comment on pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#issuecomment-788834087
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#discussion_r584388763
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -957,8 +957,9 @@ public NamespaceService getNamespaceService() {
return functionWorkerService;
}
- public WorkerService getWorkerService() {
- return functionWorkerService.orElse(null);
+ public WorkerService getWorkerService() throws UnsupportedOperationException {
+ return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Function Worker is not " +
+ "enabled, probably functionsWorkerEnabled is set to false"));
Review comment:
And please check the failed tests, there are 2 Checkstyle violations
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] golden-yang commented on a change in pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
golden-yang commented on a change in pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#discussion_r584462509
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -957,8 +957,9 @@ public NamespaceService getNamespaceService() {
return functionWorkerService;
}
- public WorkerService getWorkerService() {
- return functionWorkerService.orElse(null);
+ public WorkerService getWorkerService() throws UnsupportedOperationException {
+ return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Function Worker is not " +
+ "enabled, probably functionsWorkerEnabled is set to false"));
Review comment:
Before submitting the code, I forgot to check the code style. I have already checked and resubmitted
thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #9738: [broker] change getWorkerService method to throw UnsupportedOperationException
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9738:
URL: https://github.com/apache/pulsar/pull/9738#discussion_r584388405
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -957,8 +957,9 @@ public NamespaceService getNamespaceService() {
return functionWorkerService;
}
- public WorkerService getWorkerService() {
- return functionWorkerService.orElse(null);
+ public WorkerService getWorkerService() throws UnsupportedOperationException {
+ return functionWorkerService.orElseThrow(() -> new UnsupportedOperationException("Pulsar Function Worker is not " +
+ "enabled, probably functionsWorkerEnabled is set to false"));
Review comment:
I think you can use `getWorkerServiceOpt()` to check if the `workerService` is present? Since the `workerService` might be used in multiple places if we throw an exception here, this means all the places will throw an exception, this is not necessarily the right choice for all operations. So my suggestion is to let the caller to decide the behavior by checking the `getWorkerServiceOpt()`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org