Skip to content

Factored-out the state management in designated classes.#73

Closed
ndobromirov wants to merge 11 commits intolcache:masterfrom
ndobromirov:l1-state-manager
Closed

Factored-out the state management in designated classes.#73
ndobromirov wants to merge 11 commits intolcache:masterfrom
ndobromirov:l1-state-manager

Conversation

@ndobromirov
Copy link
Copy Markdown
Contributor

@ndobromirov ndobromirov commented Dec 9, 2016

Overview

This pull request:

  • Adds a feature
  • Needs tests - for the new classes.

Summary

Draft Implementation of #63

Description

We have a different classes that handle the L1 states.
Current implementations are: APCu, Static and NULL.
Maybe add SQLite one.

Update:
Improved in-line docks.

Based on discussion in the issue, decided to go with constructor injection for both pool and state StateL1 handler instance for all L1Cache drivers.

Added a factory to simplify the construction of any L1 instance, as we need to inject more dependencies now. Moved the POOL value generation in the factory (for now).

Re-factored tests to use the factory instead of directly instantiating the concrete L1 instances.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 9, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 16def54 on ndobromirov:l1-state-manager into 85ca4fd on lcache:master.

@davidstrauss davidstrauss added this to the v1.0.0 milestone Dec 9, 2016
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 11, 2016

Coverage Status

Coverage decreased (-1.0%) to 99.006% when pulling d513eaa on ndobromirov:l1-state-manager into 85ca4fd on lcache:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.8%) to 99.169% when pulling 085c651 on ndobromirov:l1-state-manager into 85ca4fd on lcache:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.7%) to 99.307% when pulling 8b0040e on ndobromirov:l1-state-manager into 85ca4fd on lcache:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.6%) to 99.444% when pulling 247b398 on ndobromirov:l1-state-manager into 85ca4fd on lcache:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 11, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling c4419bd on ndobromirov:l1-state-manager into 85ca4fd on lcache:master.

@ndobromirov
Copy link
Copy Markdown
Contributor Author

🎉 After some struggles with the build this seems ready for review :)
It would be nice to squash this on commit

@davidstrauss
Copy link
Copy Markdown
Member

It would be nice to squash this on commit

That's the only mode I have enabled for this project. If something requires multiple, isolated changes, I'd prefer to break them up into different pull requests, anyway.

Copy link
Copy Markdown
Member

@davidstrauss davidstrauss left a comment

Choose a reason for hiding this comment

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

I mentioned a small change, but I'd like to hold off on merging this until I finish investigating RocksDB as an L1 option. Isolating L1 metadata from L1 storage will only be necessary if we go with SQLiteL1.

src/APCuL1.php Outdated
$apcu_key = $this->getLocalKey($address);
$overhead = apcu_fetch($apcu_key . ':overhead', $success);
if ($success) {
if ($success === true) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we swap these to be true === $success here and in similar places. It just minimizes the chance of accidentally assigning instead of comparing. I'll probably fix all other conditions at some point to be this way.

…tored 2 loops in StaticL2 to reduce code indentation.
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 13, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 1fb9130 on ndobromirov:l1-state-manager into 85ca4fd on lcache:master.

@ndobromirov
Copy link
Copy Markdown
Contributor Author

I would say the separation is good to be available either way.
SQLite is more accessible / available compared to RocksDB in PHP, at least at the moment.

@davidstrauss
Copy link
Copy Markdown
Member

SQLite is more accessible / available compared to RocksDB in PHP, at least at the moment.

That's a good point. APCu isn't going to cut it for L1, so we'll want to keep something around for more basic users.

@ndobromirov ndobromirov mentioned this pull request Dec 27, 2016
@ndobromirov
Copy link
Copy Markdown
Contributor Author

PR #76 was based on this one and it is now merged, making one obsolete. Closing.

@ndobromirov ndobromirov deleted the l1-state-manager branch December 27, 2016 23:53
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.

3 participants