You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "flowchartsman (via GitHub)" <gi...@apache.org> on 2023/05/25 19:47:56 UTC
[GitHub] [pulsar] flowchartsman opened a new pull request, #20404: Go functions take static grpcPort in k8s runtime
flowchartsman opened a new pull request, #20404:
URL: https://github.com/apache/pulsar/pull/20404
Fixes #20403
### Motivation
Allow Go pulsar functions to respond to the function worker for status requests.
### Modifications
Make KubernetesRuntimeFactory assign static grpcPort instead of a random available port, so that the pod will respond correctly to status requests.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage. I didn't see any tests for this, but if needed I can add them.
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/flowchartsman/pulsar/pull/2
--
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] michaeljmarshall commented on a diff in pull request #20404: [fix][fn] Go functions need to use static grpcPort in k8s runtime
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #20404:
URL: https://github.com/apache/pulsar/pull/20404#discussion_r1206255842
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntimeFactory.java:
##########
@@ -280,6 +280,8 @@ public KubernetesRuntime createContainer(InstanceConfig instanceConfig, String c
instanceFile = pythonInstanceFile;
break;
case GO:
+ // Go functions should communicate via grpcPort directly.
+ instanceConfig.setPort(grpcPort);
Review Comment:
I agree that these added lines fix the problem, but I think the following might be a more general solution:
```java
// pass grpcPort configured in functionRuntimeFactoryConfigs.grpcPort in functions_worker.yml
if (grpcPort != null) {
instanceConfig.setPort(grpcPort);
}
```
I would add this down where the `metricsPort` changes are made on line 305 in the original file.
My primary motivation for updating the `instanceConfig` for all runtimes is that the port configuration is currently _wrong_ for all runtimes. The Java and Python runtimes are not broken because they get the port value in another part of the code base. I would prefer a general solution because it ensures that the `instanceConfig` port is the right value for every runtime, even if it isn't used.
--
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] michaeljmarshall commented on pull request #20404: [fix][fn] Go functions need to use static grpcPort in k8s runtime
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #20404:
URL: https://github.com/apache/pulsar/pull/20404#issuecomment-1571065914
Looks like two test failures are flaky and one is potentially legitimate:
```
Error: Tests run: 54, Failures: 1, Errors: 0, Skipped: 42, Time elapsed: 7.687 s <<< FAILURE! - in org.apache.pulsar.functions.runtime.kubernetes.KubernetesRuntimeTest
Error: org.apache.pulsar.functions.runtime.kubernetes.KubernetesRuntimeTest.testGolangConstructor Time elapsed: 0.029 s <<< FAILURE!
java.lang.AssertionError: expected [0] but found [4332]
at org.testng.Assert.fail(Assert.java:110)
at org.testng.Assert.failNotEquals(Assert.java:1577)
at org.testng.Assert.assertEqualsImpl(Assert.java:149)
at org.testng.Assert.assertEquals(Assert.java:131)
at org.testng.Assert.assertEquals(Assert.java:643)
at org.apache.pulsar.functions.runtime.kubernetes.KubernetesRuntimeTest.verifyGolangInstance(KubernetesRuntimeTest.java:954)
at org.apache.pulsar.functions.runtime.kubernetes.KubernetesRuntimeTest.testGolangConstructor(KubernetesRuntimeTest.java:910)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:139)
at org.testng.internal.invokers.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:47)
at org.testng.internal.invokers.InvokeMethodRunnable.call(InvokeMethodRunnable.java:76)
at org.testng.internal.invokers.InvokeMethodRunnable.call(InvokeMethodRunnable.java:11)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
at java.base/java.lang.Thread.run(Thread.java:833)
```
--
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] github-actions[bot] commented on pull request #20404: Go functions take static grpcPort in k8s runtime
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20404:
URL: https://github.com/apache/pulsar/pull/20404#issuecomment-1563424589
@flowchartsman Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `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] flowchartsman commented on a diff in pull request #20404: [fix][fn] Go functions need to use static grpcPort in k8s runtime
Posted by "flowchartsman (via GitHub)" <gi...@apache.org>.
flowchartsman commented on code in PR #20404:
URL: https://github.com/apache/pulsar/pull/20404#discussion_r1207014840
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntimeFactory.java:
##########
@@ -280,6 +280,8 @@ public KubernetesRuntime createContainer(InstanceConfig instanceConfig, String c
instanceFile = pythonInstanceFile;
break;
case GO:
+ // Go functions should communicate via grpcPort directly.
+ instanceConfig.setPort(grpcPort);
Review Comment:
Yeah, I was being too conservative. This is the right call.
--
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] codecov-commenter commented on pull request #20404: [fix][fn] Go functions need to use static grpcPort in k8s runtime
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20404:
URL: https://github.com/apache/pulsar/pull/20404#issuecomment-1575076530
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/20404?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#20404](https://app.codecov.io/gh/apache/pulsar/pull/20404?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (bf6242b) into [master](https://app.codecov.io/gh/apache/pulsar/commit/e220a5d04ae16d1b8dfd7e35cdddf43f3a43fe86?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e220a5d) will **increase** coverage by `0.31%`.
> The diff coverage is `84.61%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/20404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #20404 +/- ##
============================================
+ Coverage 72.59% 72.90% +0.31%
- Complexity 31719 31900 +181
============================================
Files 1867 1867
Lines 138523 138548 +25
Branches 15204 15210 +6
============================================
+ Hits 100555 101009 +454
+ Misses 29955 29515 -440
- Partials 8013 8024 +11
```
| Flag | Coverage Δ | |
|---|---|---|
| inttests | `24.15% <0.00%> (+0.02%)` | :arrow_up: |
| systests | `24.91% <23.07%> (-0.08%)` | :arrow_down: |
| unittests | `72.19% <84.61%> (+0.34%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pulsar/pull/20404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...s/runtime/kubernetes/KubernetesRuntimeFactory.java](https://app.codecov.io/gh/apache/pulsar/pull/20404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy9ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvZnVuY3Rpb25zL3J1bnRpbWUva3ViZXJuZXRlcy9LdWJlcm5ldGVzUnVudGltZUZhY3RvcnkuamF2YQ==) | `80.21% <50.00%> (-0.22%)` | :arrow_down: |
| [.../apache/pulsar/functions/runtime/RuntimeUtils.java](https://app.codecov.io/gh/apache/pulsar/pull/20404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy9ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvZnVuY3Rpb25zL3J1bnRpbWUvUnVudGltZVV0aWxzLmphdmE=) | `79.44% <76.92%> (+1.02%)` | :arrow_up: |
| [...pulsar/functions/instance/go/GoInstanceConfig.java](https://app.codecov.io/gh/apache/pulsar/pull/20404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy9pbnN0YW5jZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Z1bmN0aW9ucy9pbnN0YW5jZS9nby9Hb0luc3RhbmNlQ29uZmlnLmphdmE=) | `100.00% <100.00%> (ø)` | |
| [...unctions/runtime/kubernetes/KubernetesRuntime.java](https://app.codecov.io/gh/apache/pulsar/pull/20404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy9ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvZnVuY3Rpb25zL3J1bnRpbWUva3ViZXJuZXRlcy9LdWJlcm5ldGVzUnVudGltZS5qYXZh) | `38.96% <100.00%> (+0.61%)` | :arrow_up: |
... and [109 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/20404/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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 merged pull request #20404: [fix][fn] Go functions need to use static grpcPort in k8s runtime
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #20404:
URL: https://github.com/apache/pulsar/pull/20404
--
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] michaeljmarshall commented on a diff in pull request #20404: [fix][fn] Go functions need to use static grpcPort in k8s runtime
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #20404:
URL: https://github.com/apache/pulsar/pull/20404#discussion_r1206255842
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntimeFactory.java:
##########
@@ -280,6 +280,8 @@ public KubernetesRuntime createContainer(InstanceConfig instanceConfig, String c
instanceFile = pythonInstanceFile;
break;
case GO:
+ // Go functions should communicate via grpcPort directly.
+ instanceConfig.setPort(grpcPort);
Review Comment:
I agree that these added lines fix the problem, but I think the following might be a more general solution:
```
// pass grpcPort configured in functionRuntimeFactoryConfigs.grpcPort in functions_worker.yml
if (grpcPort != null) {
instanceConfig.setPort(grpcPort);
}
```
I would add this down where the `metricsPort` changes are made on line 305 in the original file.
My primary motivation for updating the `instanceConfig` for all runtimes is that the port configuration is currently _wrong_ for all runtimes. The Java and Python runtimes are not broken because they get the port value in another part of the code base. I would prefer a general solution because it ensures that the `instanceConfig` port is the right value for every runtime, even if it isn't used.
--
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] flowchartsman commented on a diff in pull request #20404: [fix][fn] Go functions need to use static grpcPort in k8s runtime
Posted by "flowchartsman (via GitHub)" <gi...@apache.org>.
flowchartsman commented on code in PR #20404:
URL: https://github.com/apache/pulsar/pull/20404#discussion_r1207014840
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntimeFactory.java:
##########
@@ -280,6 +280,8 @@ public KubernetesRuntime createContainer(InstanceConfig instanceConfig, String c
instanceFile = pythonInstanceFile;
break;
case GO:
+ // Go functions should communicate via grpcPort directly.
+ instanceConfig.setPort(grpcPort);
Review Comment:
Yeah, I was being too conservative. This is the right call. Changing the PR, as requested.
--
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] flowchartsman commented on pull request #20404: [fix][fn] Go functions need to use static grpcPort in k8s runtime
Posted by "flowchartsman (via GitHub)" <gi...@apache.org>.
flowchartsman commented on PR #20404:
URL: https://github.com/apache/pulsar/pull/20404#issuecomment-1574969947
It looks like the only failures I have now are related to Codecov 👍
--
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