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.

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

String manifest #29

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

String manifest #29

wants to merge 11 commits into from

Conversation

raboof
Copy link
Member

@raboof raboof commented Oct 19, 2015

Esp. tests probably need some more restructuring, but I quite like the simplification this gives for the case where there's a separate place for metadata (so a codec isn't needed).

@raboof
Copy link
Member Author

raboof commented Dec 1, 2015

@agemooij ping :)

@agemooij
Copy link
Contributor

agemooij commented Dec 1, 2015

I'm settings my hopes on the Christmas break for some Stamina time.

High workload + winter (max 6 hours of daylight) == bleeergh when it comes to evening/weekend motivation.

@agemooij
Copy link
Contributor

agemooij commented Dec 1, 2015

But you are right and I'll try to go through this ASAP

@agemooij
Copy link
Contributor

Rebased this Pr on top of master

@agemooij
Copy link
Contributor

Could you briefly explain the rationale of not using Persisted at the Persister level but only at the Persisters level? I'm not sure I understand why this is better since I value consistency (but perhaps too much).

@raboof
Copy link
Member Author

raboof commented Feb 19, 2016

Mostly for consistency within Persister: some of the methods really need to treat the manifest and the payload separately. It seemed odd to use Persisted for some of the methods but the separate components for others.

@raboof
Copy link
Member Author

raboof commented Feb 19, 2016

(that rebase basically made my local branch useless btw... I think I prefer just merging in master instead, and perhaps when the PR is complete do a squash+rebase once)

@agemooij
Copy link
Contributor

Ah, yeah. I thought it would be good to be based on master to see if everything still worked and I'm not sure merging master into a branch, then squashing all commits (including the merged ones), and then rebasing on master again would actually work. Have you got experience with that way of doing things?

Anyway, apologies for forcing you to re-checkout the branch!

@raboof
Copy link
Member Author

raboof commented Feb 19, 2016

No problem, in this case I don't think I had any non-commited work/branches :)

I think merge+rebase/squash should work just fine, but admittedly I usually don't rebase/squash at all :).

Copy link
Contributor

@agemooij agemooij left a comment

Choose a reason for hiding this comment

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

Let's discuss this one last time (online or offline) and then just go for it. I've lost almost all context/history and I find it hard to merge this without refreshing the original reasons for some of these changes.

Some questions:

  • can you make a quick summary of the major changes and why they are necessary to move towards StringManifest?
  • Is this PR is mergeable state? The original comment seems to indicate that tests (or testkit changes) are not finished yet?

My apologies for the excessively slow reaction times. Stamina has been off my radar for quite some time.

@raboof
Copy link
Member Author

raboof commented Dec 30, 2016

Sure! The main changes are:

  • store the bytes as Array[Byte] instead of akka.util.ByteString to save some unneccesary conversions
  • this method removes the need for Codec, as we can encode the class and version in the manifest instead of in the payload. For flexibility and (some) backwards compatibility I've kept the 'old' codec-based serializer available as CodecBasedStaminaAkkaSerializer, but made the StaminaAkkaSerializer store the class+version metadata in the manifest instead
  • changed some signatures to take a combination of Manifest and payload instead of a complete Persisted structure, as for some calls only the Manifest is needed and this seemed like a consistent way to express that

If you agree with the changes I think we (where 'we' is 'I', time permitting) should write a short migration guide for users already using Stamina without manifests, and add some further unit tests - then merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants