Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Refactored datastore session classes #112

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

Conversation

ChengyuanZhao
Copy link
Member

works on #108 ,

Did not add the documentation points from the issue initially. Please take a look at the proposed refactoring :)

@ChengyuanZhao ChengyuanZhao self-assigned this Oct 17, 2017
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

Without getting too much into the details of the PR, I would get rid of the abstract classes and replace them with interfaces. Prefer composition over inheritance. I understand that ManagerBase and StoreBase are abstract classes provided by Tomcat which you need to extend. However, KeyValuePeristentManager and KeyValuePersistentStore can extend those base classes without themselves being abstract. Instead, they can depend on implementations of the corresponding interfaces that contain the corresponding currently abstract methods. I hope this makes sense.

@ChengyuanZhao
Copy link
Member Author

@meltsufin There are a few points that I think make the inheritance way natural and practical here:

  1. we need to provide class names to tomcat for the valve, manager, and session parts, so providing interface names instead and having dependency injection on top would be awkward if even possible.
  2. these abstract classes hold some common logic that needs to be somewhere.
  3. the dogma of composition over inheritance might not be applicable here: I think probably the main supposed disadvantage of inheritance is that if requirements change in the future then the abstract parent might get a function added that the kids don't necessarily need or are inappropriate for the kids. In this instance, for example, that would mean would KeyValuePersistentSession ever have some storage-type-agnostic method added in the future that Datastore would need (but is not specific to Datastore's types) but Memcache or some other technology wouldn't? I can't see that happening; the whole point of the abstract class was to hold the logic common to all key-value storage technologies. Furthermore, if changes come from higher up in the inheritance tree from Tomcat, then we would need to make the same changes whether we used composition or inheritance in our key-value storage code anyway (since whatever approach we take would need to inherit the Tomcat types).

Where I do see an interface making sense would be perhaps at a level above key-value-persistent storage. For example, an interface for arbitrary persistent storage technologies would make sense imho since there could be functionality that a key-value based approach like Memcache doesn't have in common with other non-KV storage.

This big SO thread captures this situation pretty well (https://stackoverflow.com/questions/49002/prefer-composition-over-inheritance)

@meltsufin
Copy link
Member

@ChengyuanZhao I see that inheritance is more natural here due to how Tomcat uses these classes. Thanks for the clarification.

I'm generally fine with the change pending @balopat review and integration tests.

@meltsufin
Copy link
Member

@balopat Can you please do another pass here when you have a chance so we can close this out? Thanks!

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

Successfully merging this pull request may close these issues.

2 participants