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

Make SqlDelightWorkerTask more configurable, and update default configuration to support developing on Windows #5215

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MSDarwish2000
Copy link

@MSDarwish2000 MSDarwish2000 commented Apr 27, 2024

Migration to process isolation in #5068 resulted in broken migration verification, and plugin actions on Windows. This is due to JDBC's usage of temporary directory which cannot be retrieved from environment variables as they are, deliberately or by mistake, not passed to isolated process by default.

This PR makes SqlDelightWorkerTask more configurable by exposing properties like environment, maxHeapSize, ...etc. It also sets the "convention" value of these properties so that developing on Windows is no longer broken. It also may fix Out of memory. Java heap space exceptions on other machines.

Fixes #5129

@MSDarwish2000
Copy link
Author

MSDarwish2000 commented Apr 27, 2024

During further tests, I found that process isolation also results in

Out of memory. Java heap space

It can be fixed by passing through JvmArgs, which may result in unexpected consequences, passing predefined heap size, or reverting the process isolation.

I've opted for fixed 2GB max heap size.

Also, implementing CI tests for this situation to prevent recurrence would be great.

@MSDarwish2000 MSDarwish2000 changed the title Explicity pass environment variables to worker process Make SqlDelightWorkerTask more configurable, and update default configuration to support developing on Windows Apr 30, 2024
@MSDarwish2000
Copy link
Author

I've made the task more configurable by adding environment, systemProperties, minHeapSize, maxHeapSize, and jvmArgs. I've made environment inherit inheritable env variables from its parent similar to how DefaultJavaForkOptions handle it. Also, I've set the default value of maxHeapSize to 512MB which handles my average database well, and it is configurable for larger projects. Also, I've set defaultCharacterEncoding to "UTF-8"

A further step would be to implement ClassLoaderWorkerSpec and JavaForkOptions and implement the remaining properties, or ProcessWorkerSpec and hide the forkOptions behind an additional property.

I'd appreciate your review since this affects the API a lot.

This commit replaces `getInheritableEnvironmentVariables()` usage with a copied version of this function. I've searched online to find a cleaner solution but it seems that there is no alternative API intended for public use.
@MSDarwish2000 MSDarwish2000 force-pushed the msdarwish2000/2024-04-27/worker-env-fix branch from 37215fe to d53c9d1 Compare May 1, 2024 16:51
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.

Error in verifyCommonMainXXXDatabaseMigration task after upgrading from 2.0.1 to 2.0.2
2 participants