Skip to content

SAMZA-2516: Migrate BaseKeyValueStorageEngineFactory to be an abstract class instead of trait - #1352

Merged
cameronlee314 merged 4 commits into
apache:masterfrom
cameronlee314:base_kv_engine_scala
Jun 15, 2020
Merged

cameronlee314 merged 4 commits into
apache:masterfrom
cameronlee314:base_kv_engine_scala

Conversation

@cameronlee314

@cameronlee314 cameronlee314 commented Apr 24, 2020 •

Copy link
Copy Markdown
Contributor

Issues:

  1. If using Scala 2.11, there is a compile error when trying to use a Java class extend a Scala trait with implemented methods. BaseKeyValueStorageEngineFactory can be considered part of the Samza API, but it is a Scala trait with an implemented method, so that restricts extension.
  2. It would be good to migrate away from Scala code in general, for consistency with other new Samza code.

Changes:

  1. BaseKeyValueStorageEngineFactory is now an abstract class written in Java.
  2. Added some unit tests for BaseKeyValueStorageEngineFactory.
  3. The functionality is intended to stay the same.

Tests:

  1. Added unit tests
  2. Some tests in samza-test use in-memory key-value storage, and those tests pass.
  3. Deployed job with "drop.large.messages" enabled and verified that messages get dropped
  4. Deployed job with "disallow.large.messages" enabled and verified that exception is thrown
  5. Deployed job with changelog enabled and verified that changelog was being written to

API changes:
BaseKeyValueStorageEngineFactory is no longer a Scala trait, so that could potentially impact existing inheritors:

  1. Any existing Scala classes which uses the with syntax to mix in BaseKeyValueStorageEngineFactory will need to be updated to use BaseKeyValueStorageEngineFactory as an abstract class instead. If an existing Scala class used extends BaseKeyValueStorageEngineFactory (such as how InMemoryKeyValueStorageEngineFactory and RocksDbKeyValueStorageEngineFactory uses BaseKeyValueStorageEngineFactory), then it should not need to change.
  2. Scala 2.12 compiles traits into interfaces with default methods, so a Java class built against the Scala 2.12 version of BaseKeyValueStorageEngineFactory won't be able to use it as an interface. A Java class will need to use BaseKeyValueStorageEngineFactory as an abstract class. I suppose BaseKeyValueStorageEngineFactory could work as an interface with a default implementation for getStorageEngine, but it seems like abstract class fits the pattern better, since getKVStore shouldn't be a public method.

Upgrade instructions:
Any custom implementation of BaseKeyValueStorageEngineFactory needs to treat it as an abstract class, not a trait/interface. If it is necessary to make changes, only class inheritance structure should need to be changed. The functionality/signature of getStorageEngine and the usage of the getKVStore abstract method are the same, so those pieces should stay the same.

@PanTheMan PanTheMan left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall. I just have some minor comments

@PawasChhokra PawasChhokra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just left a few minor comments. Otherwise looks good to me.

@rmatharu-zz rmatharu-zz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

took one pass

@PanTheMan PanTheMan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@rmatharu-zz rmatharu-zz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@cameronlee314
cameronlee314 merged commit a5f7a66 into apache:master Jun 15, 2020
@cameronlee314
cameronlee314 deleted the base_kv_engine_scala branch November 17, 2021 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants