dataflow: extensible template with javascript udf #8291
Conversation
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
|
@dandhlee Oh yes, region tags were created (this morning) and added. |
There was a problem hiding this comment.
This looks great! Thanks. I made a few minor comments but overall looks good.
From a branding perspective, the non-Flex templates are called "Classic Templates" so maybe we want to rename this from extensible to classic?
| * @param {string} inJson input Pub/Sub JSON message (stringified) | ||
| */ | ||
| function process(inJson) { | ||
| var obj = JSON.parse(inJson), |
There was a problem hiding this comment.
It looks like Nashorn (partially) supports ECMA6, so let's see if we can modernize this Javascript a little bit.
Does it work if we change var to const? This forces the "variable" to be immutable, and makes it respect naming scopes.
There was a problem hiding this comment.
No, it doesn't unfortunately. Tests failed after I made the switch. Keeping var but we should file a bug.
There was a problem hiding this comment.
Yeah, let's keep var for now. Although if Nashorn is getting deprecated in the next Java version, maybe we can let it be. It looks like Java 11 will end of life in around 1 year and alongside with that Nashorn will be deprecated, the templates and other UDFs will most likely have to adapt GraalVM or something similar since Nashorn won't be around.
|
Thanks @dandhlee and @davidcavazos for the review. Addressed all the review comments. For the naming, we agreed on not updating extensible template to classic template because the correct name is actually "Google-provided templates". All "Google-provided templates" are now extensible and we can also create flex templates that are extensible. "extensible" templates aren't really a type of templates defined in the same sense as a feature but a concept. |
|
LGTM! Thank you! This looks great :) |
Toward internal b/238353202 and b/244173472 (region tags are created on Monday, Aug 29)