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 2020/12/02 08:20:01 UTC
[GitHub] [pulsar] wolfstudy opened a new pull request #8780: Fix single-quotes added to user-conf
wolfstudy opened a new pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780
Signed-off-by: xiaolong.ran <rx...@apache.org>
Fixes #8757
### Motivation
For the config content of Go Function, different runtime types require different parameters. Under Process Runtime, we only need the json string of config content itself; for k8s runtime, since we need to use the config content object to splice the command line in the pod, we need to add single quotes to the config content for processing.
### Modifications
In this pull request, we introduced a `k8sRuntime` flag label, which is used to identify different runtime forms.
----------------------------------------------------------------
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] vaihtovirta commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
vaihtovirta commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534081227
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
But we could use a similar quote escaping mechanism with JsonPrinter, couldn't we?
My concern that your solution might still produce an incorrectly escaped string if k8sruntime == true.
It'd be great to have some integration tests that verify a function that has user-config can be successfully started in K8S runtime.
----------------------------------------------------------------
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] wolfstudy commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534188985
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
Again, In go runtime, we have no way to directly parse the parameters passed in from the command line. This is limited by go instance. We converted all the configuration to `GoInstanceConfig` for processing. The reason for this problem is because in the runtime of the process , Due to the introduction of single quotation marks in config content, the problem of nesting of single quotation marks may occur when creating functions.
----------------------------------------------------------------
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] sijie merged pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780
----------------------------------------------------------------
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] flowchartsman commented on pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#issuecomment-737655533
LGTM in testing
----------------------------------------------------------------
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] wolfstudy commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534660366
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
Thanks @vaihtovirta @flowchartsman work for this, If we have a better solution, we can send an issue to track this issue.
----------------------------------------------------------------
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] wolfstudy commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534048235
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
Go Runtime is different from Python and Java Functions runtime, this is mainly the language itself. Go Runtime separately encapsulates the GoInstanceConfig object to process the data transmitted from FunctionDetails, so we can’t use the methods carried by protobuf itself to parse . In Go Runtime, we use `-instance-conf` to receive and process all parameter information.
----------------------------------------------------------------
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] wolfstudy commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534186602
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
> But we could use a similar quote escaping mechanism with JsonPrinter, couldn't we?
This library is provided by k8s, which is not suitable for Go Runtime here, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] vaihtovirta commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
vaihtovirta commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534027994
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
Why cannot we use a similar approach as for the Java and Python runtimes (`JsonFormat.printer`)?
https://github.com/apache/pulsar/blob/a045d926c9620a3744a4bc755bf6f2a65209e6b6/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java#L349
You're fixing the issue for non-k8s runtimes, but wouldn't the problem with json parsing still occur in k8s runtime?
----------------------------------------------------------------
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] vaihtovirta commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
vaihtovirta commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534379591
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
I realized JsonPrinter accepts protobuf's MessageOrBuilder, fair enough.
Still, we have to be sure that a string like `--user-config='{"foo": "bar"}'` is propagated correctly when wrapped into single quotes.
Here's a simple example:
```bash
# Current approach: single quotes in the json get removed after wrapping the clause with additional single quotes.
➜ echo '--user-config='{"foo": "bar"}''
➜ --user-config={foo: bar}
# Possible option: prefix with $ and use escaping \'
➜ echo $'--user-config=\'{"foo": "bar"}\''
➜ --user-config='{"foo": "bar"}'
```
What do you think?
----------------------------------------------------------------
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] vaihtovirta commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
vaihtovirta commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r535063408
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
@flowchartsman I see, I realized `-instance-conf` takes a json string as an argument, so my previous message doesn't make sense.
----------------------------------------------------------------
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] wolfstudy commented on pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#issuecomment-737650417
@flowchartsman Can you take a look at this change?
----------------------------------------------------------------
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] flowchartsman commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534555566
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
Having looked into it a bit more, I see now that the k8s runtime shells out to issue some `chmod` hacks and run `exec` on the go binary itself. While this seems suboptimal, I at least understand what's going on a bit better now. I was talking with @addisonj a bit about this and would it make more sense to build the arguments and then only later post process them to add quotes if the runtime is k8s?
----------------------------------------------------------------
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] flowchartsman commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534524426
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
> Still, we have to be sure that a string like --user-config='{"foo": "bar"}' is propagated correctly when wrapped into single quotes.
Why? As far as I understand it, `--user-config` must be JSON. Strings that begin with single quotes are invalid JSON, full stop. If you specify this on the command line, your shell will take care of this, and if you put it in that value on purpose, then your function will fail and that is expected behavior.
----------------------------------------------------------------
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] wolfstudy commented on pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#issuecomment-737072951
@freeznet @flowchartsman PTAL 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] flowchartsman commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534555566
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
Having looked into it a bit more, I see now that the k8s runtime shells out to issue some cmod hacks and run `exec` on the go binary itself. While this seems suboptimal, I at least understand what's going on a bit better now. I was talking with @addisonj a bit about this and would it make more sense to build the arguments and then only later post process them to add quotes if the runtime is k8s?
----------------------------------------------------------------
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] wolfstudy commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534186602
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
> But we could use a similar quote escaping mechanism with JsonPrinter, couldn't we?
This library is provided by k8s, maybe it is not suitable for Go Runtime here, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wolfstudy commented on pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#issuecomment-737244783
@sijie PTAL 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] flowchartsman commented on a change in pull request #8780: Fix single-quotes added to user-conf
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on a change in pull request #8780:
URL: https://github.com/apache/pulsar/pull/8780#discussion_r534524426
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
##########
@@ -224,11 +226,13 @@
ObjectMapper objectMapper = ObjectMapperFactory.getThreadLocal();
String configContent = objectMapper.writeValueAsString(goInstanceConfig);
- // Nit: at present, the implementation of go function depends on pulsar-client-go,
- // pulsar-client-go uses cgo, so the currently uploaded executable doesn't support cross-compilation.
args.add(originalCodeFileName);
args.add("-instance-conf");
- args.add("'" + configContent + "'");
+ if (k8sRuntime) {
Review comment:
> Still, we have to be sure that a string like --user-config='{"foo": "bar"}' is propagated correctly when wrapped into single quotes.
Why? As far as I understand it, `--user-config` must be JSON. Strings that begin with single quotes are invalid JSON, full stop. If you specify this on the command line, your shell will take care of this, and if you put it in that value on purpose, then your function will fail and it is your fault.
----------------------------------------------------------------
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