Factor out storage key generation logic, to eliminate code duplication

This is a small-ish refactoring chore. See #360 (comment)

Currently there is some code duplication between ingestion and serving for the generation and parsing of Redis keys. We multiplied this in the course of implementing Cassandra storage (#360), as it is already a large change set we didn't want to introduce additional Redis refactoring to it and thus decided to log it as a follow-up.

This could be done for Redis independent of and before integrating the Cassandra implementation though, and Cass implementation can be updated to follow the pattern if so.

Update: The below storage modularization was realized through #529 and subsequent PRs that implemented its interfaces. This issue remains for tech debt of code duplication that still remains, enumerated in #402 (comment)

~~In the interest of storage modularization to minimize dependency headaches in the future (something I believe we'll hash out in further RFC issues), I propose something like this:~~

~~1. A storage-api module, which defines an interface something like KeyUtil~~
~~1. A storage-redis implementation module which implements KeyUtil for Redis~~

~~I'm open to better naming suggestions, structure is more my focus here.~~

~~The KeyUtils could ideally be static so that unit testing them is trivially simple and the unit tests can serve as a good spec/documentation of the key format.~~

Makes a lot of sense. Tests that serve as the documentation would make this valuable.

@ches

Edit: Now that I think about it, this probably requires attention sooner rather than later.

@ches would it make sense to broaden the scope here? Your proposal is currently focused on storage, which is what this issue is about, but there are various points of duplication throughout the code base that will only get worse. I am specifically thinking about feature references and feature set references, which are somewhat related to storage but not quite.

What about the following modules

common/
   types/ # facade classes over existing types generated by protos. Adds static utility methods for now.
   storage-api/ # interfaces
   storage-redis/ # specific implementation

An example of code I would like to dedupe: https://github.com/gojek/feast/pull/486/files#diff-392f2731cafab061649b269c96458c2aR130

Not sure about naming though. types could be utils I guess, but I would want to be more specific if possible. It also relies heavily on datatypes so possibly they could be colocated?

The types module would allow us to add additional functionality to classes generated from protos, and without having to store that code in either ingestion, serving, or core.

@ches

@ches would it make sense to broaden the scope here? Your proposal is currently focused on storage, which is what this issue is about, but there are various points of duplication throughout the code base that will only get worse.

Yes I definitely think it would make sense and that it's part of a larger modularization effort/need, I just wasn't prepared to write up a more comprehensively encompassing issue at the time I filed this one 😇 Wanted to put concrete pebbles into backlog consideration as I came across them, they may bubble up to the umbrella discussion and be low-hanging fruit/good-first-issue fodder in the meantime.

IMO this could be useful interim refactoring even if we refine what the end goal looks like as time goes on, it can be a progressive step without being counterproductive or a waste of time.

In short, I think this is one task within an epic.

@ches

@ches @woop
I'm thinking of giving this a go sometime this week, hopefully with your blessing, haha. After looking around, I'm partial to flink's implementation for multi-connector support, and propose something like this:

.
├── core
├── ingestion
├── serving
└── storage
    ├── api // interfaces, not abstract classes
    │   ├── BatchRetriever.java
    │   ├── FeatureStorage.java
    │   └── OnlineRetriever.java
    ├── common // Utils common across all stores that implementations may or may not use
    │   └── retry // existing retry code
    │       ├── BackOffExecutor.java
    │       └── Retriable.java
    └── connectors // implementations
        ├── bigquery
        ├── cassandra
        └── redis
            ├── RedisFeatureStorage.java
            ├── RedisKey.java
            └── RedisOnlineRetriever.java

I'm thinking of keeping the KeyUtil separate from the API since it's specific to KV stores and not really a general need, so I don't think Feast should be opinionated about their implementation. Connectors are free to share a single KeyUtil across their Storage and Retriever implementations.

The actual methods defined in the interfaces require further thought, but just throwing this out for comments first.

LGTM!

I'm thinking of keeping the KeyUtil separate from the API since it's specific to KV stores and not really a general need, so I don't think Feast should be opinionated about their implementation.

True, though I think it's fine to have interfaces in api that not every implementation needs, but on the other hand should be conservative about promoting things there until their abstractions have some exercise.

Hypothetical KeyUtil might not end up being a good case though anyway, among other things I had thought they'd ideally be static and there's really no way to have static interfaces in Java, so maybe that's a clue it's against the grain… (not to mention "util" usually makes me feel a little ashamed of myself 😅).

Connectors are free to share a single KeyUtil across their Storage and Retriever implementations.

Definitely intended this to be true though, as the original spirit of this issue.

@ches

@ches @zhilingc Just want to confirm the state of this issue. It's slated for 0.5.0. Either we should decouple it from 0.5.0, close it, or add more work to our backlog.

Can it be closed?

So we sort of let the scope of this issue morph over time to representing the broader goals of storage modularization, but I would say the original intent of this issue still isn't resolved until:

The original title and most of the description of the issue still apply for cleaning that up I'd say. So my vote would be keep this issue, but remove it from v0.5.0 milestone if that tech debt is low priority.

I can update the description if that's how we go.

So my vote would be keep this issue, but remove it from v0.5.0 milestone if that tech debt is low priority.

Let's do this please :)

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Navigation Menu

Repository navigation

Factor out storage key generation logic, to eliminate code duplication #402

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Factor out storage key generation logic, to eliminate code duplication #402

Description

Activity

woop commented on Jan 26, 2020

woop commented on Feb 23, 2020

ches commented on Mar 3, 2020

zhilingc commented on Mar 9, 2020

ches commented on Mar 9, 2020

woop commented on May 3, 2020

ches commented on May 4, 2020

ches commented on May 4, 2020

woop commented on May 4, 2020

stale commented on Jul 14, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions