Skip to content

Conversation

@galibey
Copy link
Contributor

@galibey galibey commented Nov 7, 2022

This PR changes:

  • Introduced a new interface to Fluvio client to fetch SmartModules from a cluster (based on FluvioAdmin) Introduced fluvio-sc-schema::SmartModuleApiClient that can be used from any components to interact with smartmodules in the cluster.
  • Added re-exports for the convenience of dealing with smartmodules to fluvio and fluvio-smartengine crates so it won't be needed to add other crates.
  • Moved TransformationConfig to a common place to be re-used from other parts (connectors, smdk, cli, etc.)
  • Added chaining support to smdk test tool.

@galibey galibey requested review from sehz and tjtelan November 7, 2022 15:57
@sehz sehz added this to the 0.10.1 milestone Nov 7, 2022
Copy link

@sehz sehz left a comment

Choose a reason for hiding this comment

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

first pass


# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[features]
Copy link

Choose a reason for hiding this comment

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

need to bump crate version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// Ok(smart_module)
/// }
/// ```
pub struct FluvioSmartModule {
Copy link

Choose a reason for hiding this comment

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

This is bit ambigious . Maybe SmartModuleAdmin ? or SmartModuleManager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Renamed to SmartModuleAdmin

cfg-if = "1.0.0"
derive_builder = "0.11.0"
async-trait = "0.1.51"
flate2 = "1.0"
Copy link

Choose a reason for hiding this comment

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

should be optional and depend on smartengine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are independent. smartengine feature enables the engine on the client side (precisely, on the producer), whereas flate2 uses it to decompress SmartModule binary received from the Fluvio cluster. It can be used regardless of the producer.

Copy link

@sehz sehz left a comment

Choose a reason for hiding this comment

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

2nd pass with comments on SmartModule handling on Fluvio. Let's leverage existing API


/// Represents a SmartModule loaded into Fluvio cluster
#[derive(Debug, Default)]
pub struct SmartModule {
Copy link

Choose a reason for hiding this comment

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

Don't think we need to create another abstraction. Can we use SmartModuleMetadata or SmartModulePackage. It also has API to uncompress

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean ones from fluvio-controlpanel-metadata, they don't have a binary wasm data there.

Also, they are a sort of contract between SC and the client or SPU. But here we are dealing with public API that will eventually leak to the user's code and other language clients as well, if we expose those abstractions, we will not be able to change them as frequently as we might want.

As an alternative, if we can just make fetch method return Vec<u8> of binary data only, and come up with new abstractions later, if we have doubts now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added fluvio-sc-schema::SmartModuleApiClient as API client for internal usage instead of exposing methods on Fluvio client

Copy link
Contributor

@tjtelan tjtelan left a comment

Choose a reason for hiding this comment

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

Updates look good. It is nice to have the transformation config in a common place and to be able to test chaining with smdk

@galibey galibey requested a review from sehz November 11, 2022 14:22
Copy link

@sehz sehz left a comment

Choose a reason for hiding this comment

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

LGTM. Excellent changes. Great consolidation of SmartModule chaining

@sehz
Copy link

sehz commented Nov 11, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 11, 2022
This PR changes: 
- ~~Introduced a new interface to Fluvio client to fetch SmartModules from a cluster (based on `FluvioAdmin`)~~ Introduced `fluvio-sc-schema::SmartModuleApiClient` that can be used from any components to interact with smartmodules in the cluster.
- Added re-exports for the convenience of dealing with smartmodules to `fluvio` and `fluvio-smartengine` crates so it won't be needed to add other crates.
- Moved `TransformationConfig` to a common place to be re-used from other parts (connectors, smdk, cli, etc.)
- Added chaining support to `smdk test` tool.
@bors
Copy link

bors bot commented Nov 11, 2022

Timed out.

@galibey
Copy link
Contributor Author

galibey commented Nov 11, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 11, 2022
This PR changes: 
- ~~Introduced a new interface to Fluvio client to fetch SmartModules from a cluster (based on `FluvioAdmin`)~~ Introduced `fluvio-sc-schema::SmartModuleApiClient` that can be used from any components to interact with smartmodules in the cluster.
- Added re-exports for the convenience of dealing with smartmodules to `fluvio` and `fluvio-smartengine` crates so it won't be needed to add other crates.
- Moved `TransformationConfig` to a common place to be re-used from other parts (connectors, smdk, cli, etc.)
- Added chaining support to `smdk test` tool.
@bors
Copy link

bors bot commented Nov 11, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(smartmodule): added chaining support to smdk test [Merged by Bors] - feat(smartmodule): added chaining support to smdk test Nov 11, 2022
@bors bors bot closed this Nov 11, 2022
@ajhunyady
Copy link
Contributor

Very cool, is there documentation on how to use this functionality?

@sehz sehz assigned davidbeesley and galibey and unassigned davidbeesley Nov 13, 2022
@galibey galibey deleted the feat/add-smdk-test-chain branch November 14, 2022 11:30
@galibey
Copy link
Contributor Author

galibey commented Nov 14, 2022

Very cool, is there documentation on how to use this functionality?

Not yet, but it is planned.

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.

5 participants