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

Create native SQLite loader chain #1164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

belugabehr
Copy link

Related to #1154

I have separated all of the various ways of loading the native SQLite file into their own discrete classes.

Note that all of these classes are in their own package, therefore, I can tune the logging just for this one package to get a more clear idea of what is going on in the loading of the native file.

Also, if I want to implement something like #1140 and/or #1155, then I can create the new feature as a new class, and then provide a System property to toggle which loading mechanism I want to use "default" v.s. "cache" or something like that.

@gotson
Copy link
Collaborator

gotson commented Aug 21, 2024

hi, i don't quite see how you will solve #1140 and #1155 with this ? It seems this is just a stepping stone refactoring without any other feature ?

I am generally against that kind of preparatory work, and would prefer a PR that solves issues or brings new features.

@belugabehr
Copy link
Author

belugabehr commented Aug 21, 2024

Hello,

This moves all the code into a new package, so I can set my logger to more verbose just for this one package to get better understand where the native lib is being loaded from.

Added logging (at least DEBUG, maybe INFO) where the Native JDBC lib file is loaded from.

For the other changes, you pointed out correctly that all of these proposals are breaking backwards compatibility in subtle ways.

So, in order to preserve backwards functionality, and to put new features behind flags, it will be more simple:

if (featureFlag) {
    MyCacheBasedLoader.getInstance().loadSQLiteNative();
} else {
    DefaultSQLiteNativeLoaderChain.getInstance().loadSQLiteNative();
}

and would prefer a PR that solves issues or brings new features.

The feature here is the logging aspect and easy to add new loading functionality. Also, this is my first engagement with the project, so I am hesitant to spend the time delivering a new feature and then it gets rejected or ignored. I hope you understand that perspective :)

@gotson
Copy link
Collaborator

gotson commented Aug 21, 2024

if it's just about moving the loader into it's own package for logging purposes, you can do a PR for that without all the refactoring, which IMHO may be useful, but may be bloated also. For instance it breaks the GraalVM native image tests.

Moving SQLiteJSBCLoader into its own loader subpackage could maybe suffice for now?

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.

2 participants