eventsByTag and currentEventsByTag to use Offset #21615
Conversation
akka-ci
added
validating
|
Test FAILed. |
|
Will look into test failure |
akka-ci
added
validating
|
Test FAILed. |
|
As it is an experimental module, do we keep the binary compatibility? If we do, let me fix my code.
|
| @@ -17,7 +17,7 @@ trait CurrentEventsByTagQuery extends ReadJournal { | |||
| * is completed immediately when it reaches the end of the "result set". Events that are | |||
| * stored after the query is completed are not included in the event stream. | |||
| */ | |||
| def currentEventsByTag(tag: String, offset: Long): Source[EventEnvelope, NotUsed] | |||
| def currentEventsByTag(tag: String, offset: Offset): Source[EventEnvelope, NotUsed] | |||
There was a problem hiding this comment.
One topic for discussion is how we shall introduce this change without breaking plugins.
We can change the API, since persistence-qurey is experimental, but I think it can cause unnecessary trouble to do that in a 2.4.x. When we release Akka 2.5 we can change the API before removing the experimental label.
One way would be to add CurrentEventsByTagQuery2 with the new signatures. Then in 2.5 we make that the only trait (and rename it to CurrentEventsByTagQuery).
There was a problem hiding this comment.
Sure, added EventsByTagQuery2 and CurrentEventsByTagQuery2.
|
|
||
| trait Offset { | ||
| def hashCode: Int | ||
| def asScala: akka.persistence.query.scaladsl.Offset |
There was a problem hiding this comment.
I don't think we need different classes for scaladsl and javadsl. We can place it in akka.persistence.query package.
There was a problem hiding this comment.
OK I moved the javadsl and scaladsl offset to akka.persistence.query and combined them into one.
| throw new IllegalArgumentException("UUID " + value + " is not a time-based UUID") | ||
| } | ||
|
|
||
| override def hashCode: Int = value.hashCode |
There was a problem hiding this comment.
Offset classes are case classes, so I think equals method implementation is already given?
akka-ci
added
validating
|
Test PASSed. |
|
@patriknw thanks for the review - I updated this following your comments. Could you let me know if it looks ok now? |
| import java.util.UUID | ||
|
|
||
| trait Offset { | ||
| def hashCode: Int |
There was a problem hiding this comment.
Ah I didn't realize case classes implement hasCode automatically. So I removed it in the new commit.
|
|
||
| override def hashCode: Int = value.hashCode | ||
|
|
||
| override def toString: String = value.toString |
There was a problem hiding this comment.
why override hashCode and toString? isn't the case class enough?
There was a problem hiding this comment.
Yes, that makes more sense. I didn't realize hasCode was provided by case class, and it looks case class's toString is better than what I did - removed them in the new commit.
akka-ci
added
validating
|
Test PASSed. |
|
@johanandren please review, so that we have 2 lgtm |
|
thanks @richard-imaoka |
* EventsByTagQuery2 and CurrentEventsByTagQuery2 to keep binary compatibility
Refs #21283
As discussed in the above issue, I didn't implement other offset than long (Sequence) for LevelDB journal.
I did my best for javadsl.Offset and scaladsl.Offset, but not sure if that's the correct way... please let me know if I should do them differently.