support scala3 with slick #167
Conversation
pjfanning
commented
Jun 11, 2024
•
pjfanning
commented
- relates to pekko-projection-slick scala 3 support? #110
- the scala file changes were all due to code that wouldn't compile in Scala 3
- link validator issue is unrelated to these changes
pjfanning
requested review from
raboof,
He-Pin,
mdedetrich,
nvollmar,
samueleresca and
Roiocam
| @@ -45,7 +45,7 @@ import org.slf4j.LoggerFactory | |||
| * INTERNAL API | |||
| */ | |||
| @InternalApi | |||
| private[projection] class JdbcOffsetStore[S <: JdbcSession]( | |||
| class JdbcOffsetStore[S <: JdbcSession]( | |||
There was a problem hiding this comment.
there is an example test class in the jdocs package that Scala 3 won't allow to access this package private class
| import slick.jdbc.JdbcProfile | ||
|
|
||
| /** | ||
| * INTERNAL API | ||
| */ | ||
| @InternalApi private[projection] class SlickOffsetStore[P <: JdbcProfile]( | ||
| system: ActorSystem[_], | ||
| val db: P#Backend#Database, | ||
| val profile: P, | ||
| val databaseConfig: DatabaseConfig[P], |
There was a problem hiding this comment.
I am not sure it was a good idea to directly use config rather than db instance and profile.
Are these harmful for tests?
There was a problem hiding this comment.
That should be fine, just lost some typesafety
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
We initialized it but didn't close it, would it be better to use lazy in this field?
These changes didn't seem harmless, maybe there is a issue hidden in the shadows.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I have changed the code not to eagerly call dbConfig.db.
There was a problem hiding this comment.
I do believe that the issue should be fixed separately and not bundled into this PR
fair to me.
There was a problem hiding this comment.
can you open an issue for this? rather than a discussion, i think people want to be find it on the issues? thanks~
|
lgtm |
