Skip to content
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.

Sign up for GitHub

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

Jump to bottom

+per #18190 leveldb impl of EventsByPersistenceId query #18264

Merged
merged 1 commit into from
Aug 19, 2015
Merged

+per #18190 leveldb impl of EventsByPersistenceId query #18264

merged 1 commit into from
Aug 19, 2015

Conversation

patriknw
Copy link
Member

Not ready. Requires some cleanup and documentation.

Also tinkering if we could skip the scheduled ticks altogether and rely on the Changed notifications. However, if there are many changes all the time its good to not do the query too often.

@patriknw patriknw changed the title +=per #18190 leveldb impl of EventsByPersistenceId query +per #18190 leveldb impl of EventsByPersistenceId query Aug 19, 2015
@akka-ci akka-ci added the validating PR is currently being validated by Jenkins label Aug 19, 2015
@patriknw
Copy link
Member Author

note to self: it's also missing something for the SharedLeveldbJournal

@ktoso
Copy link
Member

ktoso commented Aug 19, 2015

Refs #18190

ktoso
ktoso reviewed Aug 19, 2015
View reviewed changes
...persistence-query/src/main/scala/akka/persistence/query/journal/leveldb/DeliveryBuffer.scala
buf foreach onNext
buf = Vector.empty
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah. I actually think this is useful enough to put somewhere in streams... I've re-implemented deliverBuf far too many times by now

Copy link
Member

Choose a reason for hiding this comment

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

(at some point, not now, maybe a ticket?)

@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Aug 19, 2015
@akka-ci
Copy link

akka-ci commented Aug 19, 2015

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3451/

ktoso
ktoso reviewed Aug 19, 2015
View reviewed changes
...y/src/main/scala/akka/persistence/query/journal/leveldb/EventsByPersistenceIdPublisher.scala
case _: Request ⇒
journal ! LeveldbJournal.SubscribePersistenceId(persistenceId)
log.info("initial request ReplayMessages for persistenceId [{}] from [{}] to [{}]", persistenceId, currSeqNo, toSequenceNr)
journal ! ReplayMessages(currSeqNo, toSequenceNr, replayMax, persistenceId, self)
Copy link
Member

Choose a reason for hiding this comment

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

yeap, much nicer indeed

@patriknw patriknw force-pushed the wip-18190-leveldb-EventsByPersistenceId-patriknw branch from bcc36af to 7f4f4bc Compare August 19, 2015 12:35
@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Aug 19, 2015
@patriknw
Copy link
Member Author

@ktoso I have updated. Please take another look. I'll continue with AllPersistenceIds.

ktoso
ktoso reviewed Aug 19, 2015
View reviewed changes
akka-docs/rst/java/code/docs/persistence/PersistenceQueryDocTest.java
@@ -97,7 +99,7 @@ void demonstrateBasicUsage() {
.getReadJournalFor("akka.persistence.query.noop-read-journal");

// issue query to journal
Source<Object, BoxedUnit> source =
Source<EventEnvelope, BoxedUnit> source =
Copy link
Member

Choose a reason for hiding this comment

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

👍

@ktoso
Copy link
Member

ktoso commented Aug 19, 2015

A few minor things but looks great, LGTM

@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Aug 19, 2015
@akka-ci
Copy link

akka-ci commented Aug 19, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3454/

@ktoso
Copy link
Member

ktoso commented Aug 19, 2015

The style of having the logic in akka-persistence has grown on me, great idea.

@patriknw
Copy link
Member Author

I have fixed your comments 5d30edb

@patriknw
Copy link
Member Author

yeah, for leveldb is feels like the right thing (right ambition level)

@akka-ci akka-ci added the tested PR that was successfully built and tested by Jenkins label Aug 19, 2015
@akka-ci
Copy link

akka-ci commented Aug 19, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3453/

@akka-ci
Copy link

akka-ci commented Aug 19, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3455/

@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Aug 19, 2015
@ktoso
Copy link
Member

ktoso commented Aug 19, 2015

LGTM, squash and merge when ready

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Aug 19, 2015
@akka-ci
Copy link

akka-ci commented Aug 19, 2015

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3460/

@akka-ci akka-ci added the needs-attention Indicates a PR validation failure (set by CI infrastructure) label Aug 19, 2015
@akka-ci
Copy link

akka-ci commented Aug 19, 2015

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3461/

@patriknw
+per #18190 leveldb impl of EventsByPersistenceId query
009d80d 
* also changed EventsByPersistenceId query type to return
Source[EventEnvelope]
@patriknw patriknw force-pushed the wip-18190-leveldb-EventsByPersistenceId-patriknw branch from c6b674f to 009d80d Compare August 19, 2015 16:59
@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Aug 19, 2015
@akka-ci
Copy link

akka-ci commented Aug 19, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3463/

patriknw added a commit that referenced this pull request Aug 19, 2015
@patriknw
Merge pull request #18264 from akka/wip-18190-leveldb-EventsByPersist…
87bc512 
…enceId-patriknw

+per #18190 leveldb impl of EventsByPersistenceId query
@patriknw patriknw merged commit 87bc512 into master Aug 19, 2015
@patriknw patriknw deleted the wip-18190-leveldb-EventsByPersistenceId-patriknw branch August 19, 2015 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@patriknw @ktoso @akka-ci