You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2021/03/12 22:00:16 UTC

[GitHub] [samza] Sanil15 opened a new pull request #1475: SAMZA-2626: Push the keySerde msgSerde null check down to RocksDbKey…ValueStorageEngine factory rather than BaseKeyValueStorageEngineFactory since not all storage engine type have serdes applicable

Sanil15 opened a new pull request #1475:
URL: https://github.com/apache/samza/pull/1475


   **Issue:** BaseKeyValueStorageEngine does a null check on keySerde and msgSerde, however this null check might only apply to some StorageEngine like RocksDbStorageEngine
   
   If we want to add a new KeyValueStorageEngine for Samza which itself handles serialization/deserialization of KeyValue messages the type of Serde you want to use with it is null. Since BaseKeyValueStorageEngine assumes serdes as non-null while constructing the KeyValueStore it throws a null / serde not found exception
   
   **Changes:** Push the non-null serde validation for key and msg serde down to RocksDbKeyValueStorageEngineFactory
   
   **Tests:** Added a unit test
   
   **API Changes:** Yes
   
   BaseKeyValueStorageEngineFactory#getKeyStore has two new serde arguments
   
   ```
    protected abstract KeyValueStore<byte[], byte[]> getKVStore(String storeName,
         `Serde<K> keySerde,`
         `Serde<V> msgSerde,`
         File storeDir,
         MetricsRegistry registry,
         SystemStreamPartition changeLogSystemStreamPartition,
         JobContext jobContext,
         ContainerContext containerContext,
         StoreMode storeMode); {
   
   }
   ```
   
   **Upgrade Instructions:** 
   
   If a user class is extending BaseKeyValueStorageEngineFactory they are required to do a null check for serde in the concrete implementation of StorageEngineFactories
   
   
   Usage Instructions: None


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] Sanil15 closed pull request #1475: SAMZA-2626: Push the keySerde msgSerde null check down to RocksDbKey…ValueStorageEngine factory rather than BaseKeyValueStorageEngineFactory since not all storage engine type have serdes applicable

Posted by GitBox <gi...@apache.org>.

Sanil15 closed pull request #1475:
URL: https://github.com/apache/samza/pull/1475


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] Sanil15 commented on pull request #1475: SAMZA-2626: Push the keySerde msgSerde null check down to RocksDbKey…ValueStorageEngine factory rather than BaseKeyValueStorageEngineFactory since not all storage engine type have serdes applicable

Posted by GitBox <gi...@apache.org>.

Sanil15 commented on pull request #1475:
URL: https://github.com/apache/samza/pull/1475#issuecomment-810532643


   Took @bharathkk suggestion, we will let the BaseKeyValueStorageEngine hold the assumption for nonnull serdes, any custom KeyValueStorageEngineFactory that does not have this assumption right must not use BaseKeyValueStorageEngineFactory since the BaseKeyValueStorageEngineFactory#restore code needs this assumption, such use cases must directly use StorageEngineFactory with KeyValueStorageEngine with respective assumptions, ContainerStorageManager should also not hold any assumptions on serdes
   
   Closing this PR, will open a followup PR add more docs to BaseKeyValueStorageEngine and remove null serde redundant checks at CSM level


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] mynameborat commented on pull request #1475: SAMZA-2626: Push the keySerde msgSerde null check down to RocksDbKey…ValueStorageEngine factory rather than BaseKeyValueStorageEngineFactory since not all storage engine type have serdes applicable

Posted by GitBox <gi...@apache.org>.

mynameborat commented on pull request #1475:
URL: https://github.com/apache/samza/pull/1475#issuecomment-799476540

I am not sure if the approach taken in the PR is the ideal way to go about achieving the changes you need. I see few problems with delegating `null` checks from `BaseKeyValueStorageEngineFactory` to the extenders.

1. In `BaseKeyValueStorageEngineFactory`, we do multiple layers of wrapping stores (logged store, access logged store, serialized store) and so on.. By removing the checks, you are breaking this invariant of non-null `Serde` to all of steps that follow construction of the higher layer store (RocksDb or InMemory etc).
2. Particularly, this is problematic for the construction of `SerializedKeyValueStore` and `AccessLoggedStore` because the former assumes both `KeySerde` and `ValueSerde` to be null and the latter assumes `KeySerde` to be non-null.
3. Forces high layer `StorageEngineFactory` to handle `KeySerde` and `ValueSerde` during construction even though it is not necessary. For e.g. `RocksDbKeyValueStorageEngineFactory` and `InMemoryKeyValueStorageEngineFactory`
4. Introduces potential problems with `Serde`; i.e. Higher layer storage engines mistakenly use the `Serde` themselves and also have wrapped store handles which potentially does `Serde` as well.

It will be good if you can take into consideration the following as part of your approach

- Do storage engines mandate over what kind of wrapping is allowed? e.g. logged store and/or serialized store etc
- Do end users continue to describe properties of store that indirectly determines the wrapping applied? e.g. large message enabled store (vs cached store vs access logged store
- Does the framework determine the combination of configurations that drive the wrapping strategy?

Potential solutions I can think of
1. Introduce internal flag or have explicit store properties that drives how wrapping is accomplished and determines if a store is `SerializedKeyValueStore` or not
2. Instead of removing the null check, push it down within `BaseKeyValueStorageEngineFactory` and modify how wrapping is accomplished either based on storage engines.
3. If flexibility is the key, introduce capability for storage engines to implement that handles raw store without any these goodies and wrapping so it has all the control it needs.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org