-
Notifications
You must be signed in to change notification settings - Fork 645
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
Google Cloud Pub-Sub: towards Alpakka 1.0 #1258
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A couple of minor comments.
import scala.collection.immutable | ||
import scala.collection.JavaConverters._ | ||
|
||
class PubSubConfig(val projectId: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the default constructor private as the user API is the apply method.
clientEmail: String, | ||
privateKey: String, | ||
http: => HttpExt) { | ||
def this(projectId: String, apiKey: String, clientEmail: String, privateKey: String)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not needed. The Http extension access can be moved to the apply method.
68f7215
to
7f5d36c
Compare
Okay! I created like this bcs the java code now needs to use Amended. |
For Java API create a alpakka/reference/src/main/scala/akka/stream/alpakka/reference/settings.scala Lines 93 to 98 in 7404ed6
|
7f5d36c
to
1eb96f5
Compare
Ahh I missed that note! Thx. Fixed! |
import scala.collection.immutable | ||
import scala.collection.JavaConverters._ | ||
|
||
private[pubsub] class PubSubConfig(val projectId: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made the whole class private to the package, which hides it from the users. Instead make the default constructor private:
class PubSubConfig private (val projectId: String,
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! I saw this as a pattern somewhere, and I could use this in the docs.examples package without a problem. If the val-s are protected by default I think thats not a problem.
I think the common user will never want to use this class. Just create one instance with the given apply/create and pass it as a value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package private private[...]
does not apply when used from Java. So maybe that usage you noticed was from Java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay! Today I played with it, and I think it's not worth it. If I make that constructor private I can't tast the code. I can't extend from that class anymore so I can't change the session to a mock session.
On the other hand I tried to use the new keyword from java. For that, you need a Http.apply(actorSystem)
which is not really compile, so if you really want to hack through yourself with the constructor you need to do something like Http.createExtension((ExtendedActorSystem) system)
which is really ugly and I think it's rings some bells in any developers head.
Do you have any other idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could open up access to the session in a different way if that is needed for testing. Instead of public constructor, we could add a withSession(...)
builder method that would replace the session with the given one. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sooo we open up a protected/package-private builder method just for the testing, and with that we could hide the constructor.
Here if we use an apply and a protected(?) withSession the concept will work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. It does not look the best, I agree. Lets be explicit why providing custom session is public by creating a "testkit" factory, that would allow custom session parameter. Similar to the pattern in the reference connector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay;
class PubSubConfig private (val projectId: String,
val apiKey: String,
clientEmail: String,
privateKey: String,
inSession: GoogleSession) {
@InternalApi
protected[pubsub] lazy val session = inSession
}
object PubSubConfig {
protected[pubsub] def apply(projectId: String,
apiKey: String,
clientEmail: String,
privateKey: String,
session: GoogleSession) =
new PubSubConfig(projectId, apiKey, clientEmail, privateKey, session)
...
Works halfway... Now the constructor is private, but if I write down the val keyword before session, an auto getter is generated which will leak the session (java only). If this is not a problem, I can push my code (without the session reassign).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh. Changing to private val session
should do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if its private I can't access it from the pubsub package. So the circle is closed :( I need it to be unseeable, unmodifyable from outside the pubsub package, but I want to use it as a normal parameter inside. I think java can't do this, or I still didn't see the full picture... Either way I would be pleased if you would try to play with this for a bit and probably commit a "suitable way of doing this" example, so in the future I can use that as a reference.
I played around with the API a bit, and I think having |
Nice! Thanks. |
06b5630
to
e6638bb
Compare
e6638bb
to
4ef7835
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fixes #1074
move to impl package, flag internal, move documentation classes - done
I refactored the code, broke the api (now we cache the session in the config, and use 1 config object instead of 4 parameters).
I tried to make the config bincomp-compatible, but there are other model classes. I don't know if those are ok or not. (in the model file)
(I also refactored the session and token api code to be almost the same as the fcm and the (in progress) bigquery one, we slowly reach the point when those classes could be generalized and move to a separate subproject/repo.)