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][fn] Go functions need to use static grpcPort in k8s runtime #20404

Merged
merged 4 commits into from
Jun 5, 2023

Conversation

flowchartsman
Copy link
Contributor

@flowchartsman flowchartsman commented May 25, 2023

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:

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

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: flowchartsman#2

@github-actions
Copy link

@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 -->

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels May 25, 2023
@flowchartsman flowchartsman changed the title Go functions take static grpcPort in k8s runtime [fix][fn] Go functions need to use static grpcPort in k8s runtime May 26, 2023
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

Great work on this @flowchartsman, thanks for your PR! I agree with the direction, but I have just one detail to discuss before we move forward.

@michaeljmarshall
Copy link
Member

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)

@flowchartsman
Copy link
Contributor Author

It looks like the only failures I have now are related to Codecov 👍

@codecov-commenter
Copy link

Codecov Report

Merging #20404 (bf6242b) into master (e220a5d) will increase coverage by 0.31%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             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%) ⬆️
systests 24.91% <23.07%> (-0.08%) ⬇️
unittests 72.19% <84.61%> (+0.34%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/runtime/kubernetes/KubernetesRuntimeFactory.java 80.21% <50.00%> (-0.22%) ⬇️
.../apache/pulsar/functions/runtime/RuntimeUtils.java 79.44% <76.92%> (+1.02%) ⬆️
...pulsar/functions/instance/go/GoInstanceConfig.java 100.00% <100.00%> (ø)
...unctions/runtime/kubernetes/KubernetesRuntime.java 38.96% <100.00%> (+0.61%) ⬆️

... and 109 files with indirect coverage changes

@lhotari lhotari merged commit 7e6ca31 into apache:master Jun 5, 2023
michaeljmarshall pushed a commit that referenced this pull request Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Unable to communicate with Go Pulsar functions using the Kubernetes runtime.
5 participants