Skip to content

Feature/shadding #379

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

Merged
merged 1 commit into from
Jun 24, 2020
Merged

Feature/shadding #379

merged 1 commit into from
Jun 24, 2020

Conversation

amamounelsayed
Copy link
Contributor

@amamounelsayed amamounelsayed commented Jun 17, 2020

This feature is for shading all worker jars.

Only for JAVA 8
To be a backward compatible I freeze all the jars we currently have added to repo.
Then when we load the classloader we will load the worker lib first then customer.

If the user like to reverse this order and their jars takes presence first, they will need to add FUNCTIONS_WORKER_JAVA_LOAD_APP_LIBS True or 1. By this way it will be opt-in scenario.

JAVA 11 will use only the customer jars.

Generated jar after shading:
azure-functions-java-worker.zip

Fixes #340, Fixes #183, Fixes #373, Fixes #368, Fixes #365, Fixes #341

@amamounelsayed amamounelsayed changed the title Feature/shadding [work in progress] Feature/shadding Jun 17, 2020
@amamounelsayed amamounelsayed force-pushed the feature/shadding branch 2 times, most recently from 6ea52af to 83d5a08 Compare June 18, 2020 05:29
@amamounelsayed amamounelsayed changed the title [work in progress] Feature/shadding Feature/shadding Jun 19, 2020
Copy link
Member

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

Thanks! Added few comments.

Copy link
Contributor

@TsuyoshiUshio TsuyoshiUshio left a comment

Choose a reason for hiding this comment

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

I reviewed the PR. Could you have a look?

pragnagopa
pragnagopa previously approved these changes Jun 19, 2020
Copy link
Member

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

LGTM. Added more comments. Please wait for sign off from @TsuyoshiUshio as well.

@pragnagopa
Copy link
Member

One more question: instead of checking in individual jars, is it possible to ship one jar file?

@amamounelsayed
Copy link
Contributor Author

amamounelsayed commented Jun 19, 2020

One more question: instead of checking in individual jars, is it possible to ship one jar file?

Got it, this may add more complication to the jar loading. I can have a look but this approach is just temporary solution.

@pragnagopa
Copy link
Member

I am sorry did not get the question.

This PR includes individual jar files in folder lib_worker_1.6.2, can we combine them into one jar file: libWorker1.6.2.jar : https://stackoverflow.com/questions/5080220/how-to-combine-two-jar-files

@amamounelsayed
Copy link
Contributor Author

This PR includes individual jar files in folder lib_worker_1.6.2, can we combine them into one jar file: libWorker1.6.2.jar : https://stackoverflow.com/questions/5080220/how-to-combine-two-jar-files

Got it, this may add more complication to the jar loading. I can have a look but this approach is just temporary solution.

@pragnagopa
Copy link
Member

Got it, this may add more complication to the jar loading. I can have a look but this approach is just temporary solution.

I missed mentioning this before, can you please update PR description with size of the java worker folder with this change? If it is significantly higher, it is worth combining jars and checking the size. Due to size restrictions on windows VMs, we need to ensure worker size does not change much with this change.

@amamounelsayed
Copy link
Contributor Author

I missed mentioning this before, can you please update PR description with size of the java worker folder with this change? If it is significantly higher, it is worth combining jars and checking the size. Due to size restrictions on windows VMs, we need to ensure worker size does not change much with this change.

The nuget will be increased by around 12MB.

@pragnagopa
Copy link
Member

The nuget will be increased by around 12MB.

Thanks. Not a blocker then!

@pragnagopa
Copy link
Member

Does this PR address: ##340 and #183 ?

@amamounelsayed
Copy link
Contributor Author

Thank you so much @pragnagopa for your time.

Does this PR address: ##340 and #183 ?

Yeap this is correct also
#373
#368
#365
#341

@pragnagopa
Copy link
Member

Updated the description to include references to issues to ensure merging PR closed all of them.

TsuyoshiUshio
TsuyoshiUshio previously approved these changes Jun 24, 2020
Copy link
Contributor

@TsuyoshiUshio TsuyoshiUshio left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@TsuyoshiUshio TsuyoshiUshio left a comment

Choose a reason for hiding this comment

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

LGTM

@amamounelsayed amamounelsayed merged commit 5910167 into dev Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment