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: use args map instead of method args in generated java code #9711

Merged
merged 1 commit into from Feb 10, 2023
Merged

fix: use args map instead of method args in generated java code #9711

merged 1 commit into from Feb 10, 2023

Conversation

Nick-VdP
Copy link
Contributor

@Nick-VdP Nick-VdP commented Nov 16, 2022

Description

Fixes #9353

When generating java from ksql, all arguments are passed as individual method arguments. The java compiler only allows up to 255 method arguments. After a ksql statement reaches a certain size, this limit is nolonger sufficient resulting in the following ClassFormatError:

Exception in thread "main" java.lang.ClassFormatError: Too many arguments in method signature in class file SC at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017) at org.codehaus.janino.ByteArrayClassLoader.findClass(ByteArrayClassLoader.java:74) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:589) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at org.codehaus.janino.ClassBodyEvaluator.compileToClass(ClassBodyEvaluator.java:317) at org.codehaus.janino.ScriptEvaluator.cook2(ScriptEvaluator.java:608) at org.codehaus.janino.ScriptEvaluator.cook(ScriptEvaluator.java:597) at org.codehaus.janino.ScriptEvaluator.cook(ScriptEvaluator.java:534) at org.codehaus.janino.ScriptEvaluator.cook(ScriptEvaluator.java:503) at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:204) at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:80) at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:75) at io.confluent.ksql.execution.codegen.CodeGenRunner.cook(CodeGenRunner.java:195) ...

This change puts all those arguments in one map. This map is then passed as one method argument.

Testing done

Unit tests have been changed to reflect the changes made in this PR.

A new test 'shouldHandleManyArguments' has been added to SqlToJavaVisitorTest.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2022

CLA assistant check
All committers have signed the CLA.

@Nick-VdP Nick-VdP marked this pull request as ready for review November 16, 2022 15:32
@Nick-VdP Nick-VdP requested a review from a team as a code owner November 16, 2022 15:32
@Nick-VdP
Copy link
Contributor Author

I'm wondering If there is anything else I need to do to get this pull request merged. Can someone from Confluent please take a look at this?

Copy link
Member

@stevenpyzhang stevenpyzhang left a comment

Choose a reason for hiding this comment

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

lgtm, with build passing

@aliehsaeedii
Copy link
Contributor

Thanks @Nick-VdP for your contribution. Do you please do a rebase and build the PR locally?

@Nick-VdP
Copy link
Contributor Author

Nick-VdP commented Feb 7, 2023

@aliehsaeedii Rebased, built and tested locally.

Jenkins-public-CI doesn't seem to be able to finish in time and aborts after a timeout.

@aliehsaeedii
Copy link
Contributor

@aliehsaeedii Rebased, built and tested locally.

Jenkins-public-CI doesn't seem to be able to finish in time and aborts after a timeout.

Thanks @Nick-VdP.
Did you @cadonna increase the timeout?

@cadonna
Copy link
Member

cadonna commented Feb 10, 2023

@Nick-VdP @aliehsaeedii The timeout for the PR builder was fixed. Now the timeout should be 5 hours as specified in the Jenkinsfile.
In the meanwhile, we build this PR on our internal Jenkins and it was successful. Thus, I am going to merge this PR.
Sorry for the inconvenience!

@cadonna cadonna merged commit d5b6dd3 into confluentinc:master Feb 10, 2023
@Nick-VdP Nick-VdP deleted the fix/9353 branch February 10, 2023 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too many arguments in method signature in class file SC while deploying ksql stream
5 participants