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

Refactor: to explicit Java (driver) class name loading #96

Merged
merged 5 commits into from Dec 14, 2021

Conversation

kares
Copy link
Contributor

@kares kares commented Dec 13, 2021

This is a second attempt on fixing #83 as it seems to still manifest even with locking.

Few more observations on what's going on (with #83):

  • Sequel::JDBC.load_driver ends up doing an eval on the input (e.g. Java::oracle.jdbc.driver.OracleDriver)

  • the eval sometimes fails with a NameError (having multiple drivers and executing threads is a factor)

  • as hinted previously the problem seems to be specific to 'com.sybase.jdbc4.jdbc.SybDriver' driver (+ having another driver such as 'oracle.jdbc.driver.OracleDriver' around as well)

  • JRuby might have an underlying bug with eval and setting up the Java package structure for the drivers

  • at one point an ugly error came out - which indicates an underlying Java driver loading issue (although this might have been caused by the concurrent loading of different drivers - registering with DriverManager)

  • the issue only happens when drivers are on the LS class-path (setting jdbc_driver_library => did not reproduce!)

  • with the changes in this PR the issue did no longer reproduce even if drivers were on the LS class-path

A bit simplified reproducer (does not need to run Logstash): https://gist.github.com/kares/850767d4be4d5323b068af9585e45d4a

p.s. Previous changes (from #84) could be reverted but I am keeping them in (they do not hurt - the lock just slows down multiple jdbc plugins starting), for now.


More investigation might be needed for an actual underlying cause (as well as the potential JRuby bug) but the changes presented here make sense regardless - skipping eval and doing a simply Class.forName is very similar to what an actual Java JDBC application would attempt to do (and should hopefully make debugging the problem easier).

@kares kares marked this pull request as ready for review December 14, 2021 12:32
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM
The red CI is due to logstash-plugins/.ci#31

Nice idea the one don't use the Sequel load_driver method and go straight to the Java way of loading.

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.

None yet

3 participants