Skip to content

support scala3 with slick#167

Merged
pjfanning merged 7 commits intoapache:mainfrom
pjfanning:scala3
Jun 16, 2024
Merged

support scala3 with slick#167
pjfanning merged 7 commits intoapache:mainfrom
pjfanning:scala3

Conversation

@pjfanning
Copy link
Member

@pjfanning pjfanning commented Jun 11, 2024

@pjfanning pjfanning marked this pull request as draft June 11, 2024 11:18
@pjfanning pjfanning marked this pull request as ready for review June 11, 2024 19:07
@pjfanning pjfanning changed the title try to support scala3 with slick support scala3 with slick Jun 11, 2024
*/
@InternalApi
private[projection] class JdbcOffsetStore[S <: JdbcSession](
class JdbcOffsetStore[S <: JdbcSession](
Copy link
Member Author

Choose a reason for hiding this comment

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

there is an example test class in the jdocs package that Scala 3 won't allow to access this package private class

Copy link
Member

@He-Pin He-Pin 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
Member

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

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

LGTM

system: ActorSystem[_],
val db: P#Backend#Database,
val profile: P,
val databaseConfig: DatabaseConfig[P],
Copy link
Member

@Roiocam Roiocam Jun 12, 2024

Choose a reason for hiding this comment

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

I am not sure it was a good idea to directly use config rather than db instance and profile.

Are these harmful for tests?

Copy link
Member

Choose a reason for hiding this comment

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

That should be fine, just lost some typesafety

Copy link
Member Author

Choose a reason for hiding this comment

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

val db: P#Backend#Database does not compile in Scala 3 and I couldn't find any alternative that compiles either.

These classes are internal classes. I don't see it being a problem changing them. The constructor was like this before but was changed.

Copy link
Member

Choose a reason for hiding this comment

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

yes, I think this change is fine.

this(system, databaseConfig, slickSettings, Clock.systemUTC())

private[projection] val profile: P = databaseConfig.profile
private val db = databaseConfig.db
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The db instance is used instantly in the class. So I don't think making it lazy will help.
There is no lifecycle management in these classes so I don't know where we can add something to close the db instance. If anyone has any ideas that would be great.

Copy link
Member

Choose a reason for hiding this comment

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

my second link maybe helpful, it uses the system shutdown hook for instance closing.

I think they only use it when OffsetStore method call? Otherwise it only created but won't be used.

I mean the config.db is a method call too... you could lazily call it. Kind of like we got the method like:

< private Database db = instanceDB(); // initialize immediately
> private Database db; // won't initialize in creation.


> public getDB(){
>  if (db == null){
>     db = instanceDB(); // initialize here.
>  }
>  return db;
> }

public void something_operation() {
< return db.call();
> return getDB().call();
}


Copy link
Member Author

Choose a reason for hiding this comment

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

I've created #168

  • this PR does not introduce the db close issue - it is a pre-existing issue
  • it is also a complicated pre-existing issue - and I believe that there is no simple solution
  • I do believe that the issue should be fixed separately and not bundled into this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed the code not to eagerly call dbConfig.db.

Copy link
Member

Choose a reason for hiding this comment

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

I do believe that the issue should be fixed separately and not bundled into this PR

fair to me.

Copy link
Member

Choose a reason for hiding this comment

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

can you open an issue for this? rather than a discussion, i think people want to be find it on the issues? thanks~

Copy link
Member Author

Choose a reason for hiding this comment

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

created #172

@laglangyue
Copy link
Contributor

lgtm

@pjfanning pjfanning merged commit 0afbf60 into apache:main Jun 16, 2024
@pjfanning pjfanning deleted the scala3 branch June 16, 2024 22:56
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.

5 participants