-
Notifications
You must be signed in to change notification settings - Fork 638
Couchbase: Updates the integration to support collections #3396
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @chedim, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Akka Contributors License Agreement: |
Working on it. Hopefully, it'll not take long 🤞 |
...hbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseCollectionSessionImpl.scala
Outdated
Show resolved
Hide resolved
|
Hi @chedim, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Akka Contributors License Agreement: |
|
We have received the CLA. |
|
We signed the CLA. Authorized logins: chedim and deniswrosa |
|
Closing and re-opening to trigger the CLA validator again. |
|
@ennru @sebastian-alfers @leviramsey Could somebody please review the PR whenever they get a chance? Thank you! |
patriknw
left a comment
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.
A few initial comments/questions...
...hbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseCollectionSessionImpl.scala
Outdated
Show resolved
Hide resolved
...hbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseCollectionSessionImpl.scala
Outdated
Show resolved
Hide resolved
...hbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseCollectionSessionImpl.scala
Outdated
Show resolved
Hide resolved
...hbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseCollectionSessionImpl.scala
Outdated
Show resolved
Hide resolved
couchbase/src/main/scala/akka/stream/alpakka/couchbase/scaladsl/CouchbaseFlow.scala
Outdated
Show resolved
Hide resolved
|
@chedim We will likely do an Alpakka release in the near future, is this something you would be able to get back to? |
99e1633 to
982701c
Compare
|
@ennru , Updated the PR according to comments — improved method signatures, replaced tuples with CouchbaseDocument and put futureFlows as requested. Please re-review and LMK if any additional changes are needed. Thank you! |
|
I pushed a few formalities forward, but there are even compilation errors. The docs still try to include snippets that don't exist in your new implementation The last obvious bit is MiMa, which triggers as the API changed. That is OK and we can add filters to silence it. |
|
Looking into the compilation issues, sorry for missing them. |
884c976 to
55c35fc
Compare
|
HI @ennru, I've updated the PR — compilation errors were on jdk11 (and I was running tests with jdk24, silly me). I've also updated the documentation and docker-compose definitions for ci so that they use newer CB images and create required for tests collections. Tested under jdk11 with please lmk if there are any suggestions or changes you'd like me to make. Thank you! |
|
Updated the PR with |
patriknw
left a comment
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.
looking good, only a few minor things
couchbase/src/main/scala/akka/stream/alpakka/couchbase/CouchbaseDocument.scala
Outdated
Show resolved
Hide resolved
| .clusterFor(key.settings) | ||
| .flatMap(cluster => CouchbaseSession(cluster, key.bucketName)(blockingDispatcher))( | ||
| .flatMap(cluster => CouchbaseSession(cluster, key.bucketName))( | ||
| ExecutionContext.parasitic |
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.
It's important to only use ExecutionContext.parasitic when it's absolutely certain that there will be no exceptions. That might not be the case here? createClusterClient?
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've returned the code that was using blockingDispatcher, please lmk if you want to use something else here.
Thank you!
| * @tparam T type of the object to return | ||
| * @return a future that completes with created object or errors if the operation failed | ||
| */ | ||
| def get[T](id: String, target: Class[T]): Future[CouchbaseDocument[T]] |
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.
In Scala it would be more idiomatic to use:
def get[T: ClassTag](id: String): Future[CouchbaseDocument[T]]
If you need the actual class of that ClassTag you can use:
val clazz = implicitly[ClassTag[T]].runtimeClass
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 went ahead and applied this suggestion everywhere I was using generics, please lmk if that's not what you meant.
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.
Perhaps that was going too far. I haven't looked at all usages, but in general it should only be used when you actually need the Class or don't have the T as another parameter. This get[T] is a good example. I wouldn't use it as a general replacement for [T].
Also, it must not leak to any javadsl, since this is a Scala only thing.
couchbase/src/main/scala/akka/stream/alpakka/couchbase/scaladsl/CouchbaseFlow.scala
Outdated
Show resolved
Hide resolved
154ceab to
ec0bbed
Compare
|
@patriknw , updated the PR according to the comments, had couple of questions if I understood the comments correctly. |
|
A compiler warning in the example needs to be avoided: |
|
Should be fixed now. |
johanandren
left a comment
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.
Did a round of reviewing with some feedback
...hbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseCollectionSessionImpl.scala
Outdated
Show resolved
Hide resolved
...hbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseCollectionSessionImpl.scala
Outdated
Show resolved
Hide resolved
...rc/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseCollectionSessionJavaAdapter.scala
Outdated
Show resolved
Hide resolved
couchbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseSessionImpl.scala
Outdated
Show resolved
Hide resolved
couchbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseSessionImpl.scala
Outdated
Show resolved
Hide resolved
couchbase/src/main/scala/akka/stream/alpakka/couchbase/scaladsl/CouchbaseSession.scala
Outdated
Show resolved
Hide resolved
couchbase/src/main/scala/akka/stream/alpakka/couchbase/scaladsl/CouchbaseSession.scala
Outdated
Show resolved
Hide resolved
couchbase/src/main/scala/akka/stream/alpakka/couchbase/scaladsl/CouchbaseSession.scala
Outdated
Show resolved
Hide resolved
couchbase/src/main/scala/akka/stream/alpakka/couchbase/scaladsl/CouchbaseSession.scala
Outdated
Show resolved
Hide resolved
couchbase/src/main/scala/akka/stream/alpakka/couchbase/CouchbaseDocument.scala
Outdated
Show resolved
Hide resolved
…se dependencies; Tests are passing but I need to add more coverage. # Conflicts: # couchbase/src/main/scala/akka/stream/alpakka/couchbase/impl/RxUtilities.scala # couchbase/src/main/scala/akka/stream/alpakka/couchbase/scaladsl/DiscoverySupport.scala
- Fixes a compilation error under jdk11; - updates unit tests to work with docker-compose.yaml docker couchbase instance; - updates couchbase_prep compose service to create required collection - updates to documentation related to the new APIs
be5e5a4 to
6c0068d
Compare
johanandren
left a comment
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.
Almost good to go now, good work! I left a comment about doc code snippets, please take a look at that.
| // #upsertWithResult | ||
| // #upsertWithResult |
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.
These pairs of tags in comments are there to include partial set of imports into the docs, I think to make sure the docs can be copy pasted, would be good if you could generate docs and update so that the needed imports for each snippet is included.
Generate docs and browse locally through docs/paradoxBrowse
couchbase/src/main/scala/akka/stream/alpakka/couchbase/impl/CouchbaseSessionImpl.scala
Outdated
Show resolved
Hide resolved
|
Need help! (and then it fails because it can't find these). |
|
@chedim Where is your akka.sbt file located, and what is in there? To confirm, which command are you running? |
|
sbt build my akka.sbt is in the project root. |
|
@chedim Can you confirm the directory you are in? You should run all commands from the root directory (not any sub directory). Also, the commands you share do not work for me. I am using: |
|
I'm running it from the root project directory. I re-verified my token and copy-pasted the This is the whole error when trying to update sbt project in IJ: Seems eerily similar to this issue? akka/akka-grpc#2073 |
|
Can you try to put the resolver file into |
|
That worked, thank you @sebastian-alfers , will update the PR soon. |
References #3395
This PR introduces new
CouchbaseCollectionSessionclass that can be accessed fromCouchbaseSessionviaCouchbaseSession::collectionmethod. It also updates sinks, sources and flow constructors and their signatures to work with collections.All of these are breaking changes.
Please note that, although fascinated by the framework and some language features, I am not very familiar with either alpakka or scala and more of a Java dev. So, any suggestions on how to improve my changes are greatly appreciated.
Cheers!