Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Merged by Bors] - SPU: Apply SmartModule Transformation For Producer #3014

Closed
wants to merge 52 commits into from
Closed

[Merged by Bors] - SPU: Apply SmartModule Transformation For Producer #3014

wants to merge 52 commits into from

Conversation

crajcan
Copy link
Contributor

@crajcan crajcan commented Feb 25, 2023

@crajcan crajcan marked this pull request as draft February 25, 2023 14:25
@crajcan crajcan changed the title WIP: CLI: Add SmartModule Support to Produce Command [WIP] CLI: Add SmartModule Support to Produce Command Feb 25, 2023
@crajcan crajcan force-pushed the apply_smart_module_for_producer branch from 06b2985 to 575b00c Compare March 3, 2023 19:26
@crajcan crajcan changed the title [WIP] CLI: Add SmartModule Support to Produce Command [WIP] SPU: Apply SmartModule Transformation For Producer Mar 3, 2023
@crajcan crajcan force-pushed the apply_smart_module_for_producer branch from 4b6188d to 0d0b45e Compare March 6, 2023 20:45
@crajcan crajcan force-pushed the apply_smart_module_for_producer branch 4 times, most recently from 6b5ffed to b93ebda Compare March 21, 2023 14:56
@crajcan crajcan marked this pull request as ready for review March 21, 2023 15:31
@crajcan crajcan marked this pull request as draft March 21, 2023 17:19
@crajcan crajcan marked this pull request as ready for review March 22, 2023 17:07
@crajcan crajcan changed the title [WIP] SPU: Apply SmartModule Transformation For Producer SPU: Apply SmartModule Transformation For Producer Mar 22, 2023
@crajcan crajcan force-pushed the apply_smart_module_for_producer branch 3 times, most recently from a8a3581 to 8072b30 Compare March 22, 2023 17:45
@crajcan crajcan mentioned this pull request Mar 27, 2023
Closed
@crajcan
Copy link
Contributor Author

crajcan commented Mar 27, 2023

I think the next set of failures are due to the DEV spu trying to decode smartmodule invocations that the STABLE cli doesn't send.

I'll have to tinker with the versioning so that the new spu version can support older cli versions.

@morenol
Copy link
Contributor

morenol commented Mar 27, 2023

I think the next set of failures are due to the DEV spu trying to decode smartmodule invocations that the STABLE cli doesn't send.

I'll have to tinker with the versioning so that the new spu version can support older cli versions.

I think that this comment will help you #3014 (comment)

galibey
galibey suggested changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@galibey galibey left a comment

Choose a reason for hiding this comment

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

Great job! Thanks for your contribution. Just a couple of minor fixes that are needed to be done before the merge.

crates/fluvio-spu/src/services/public/produce_handler.rs Outdated Show resolved Hide resolved
crates/fluvio-spu/src/services/public/produce_handler.rs Outdated Show resolved Hide resolved
crates/fluvio-spu-schema/src/produce/request.rs Outdated Show resolved Hide resolved
@crajcan crajcan force-pushed the apply_smart_module_for_producer branch from f404c29 to 03bb8bb Compare April 6, 2023 17:03
@crajcan crajcan force-pushed the apply_smart_module_for_producer branch from 03bb8bb to 9bd75c3 Compare April 6, 2023 17:05
sehz
sehz requested changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@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.

Looks good. Just have a minor request

.github/workflows/ci.yml Show resolved Hide resolved
crates/fluvio-spu/src/smartengine/produce_batch.rs Show resolved Hide resolved
sehz
sehz reviewed Apr 7, 2023
View reviewed changes
crates/fluvio-protocol/Cargo.toml Outdated
@@ -1,7 +1,7 @@
[package]
name = "fluvio-protocol"
edition = "2021"
version = "0.9.0"
version = "0.10.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this need major bump since we are only adding new API?

Copy link
Contributor Author

@crajcan crajcan Apr 7, 2023 •
edited

Choose a reason for hiding this comment

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

My understanding is that what I did here is a minor bump.

I think you are right though. The SemVer docs say once the major version is >0, API additions require a minor version bump.

But since the major version is still 0, I think it is still subjective. I can change these all to only increment the patch value instead, if you like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's change to only patch version which should minimize impact to downstream crates

sehz
sehz requested changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@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.

Everything looks ok except need to bump major version. Can you check if they are really needed or just need patch version upgrade if just adding fields or API

sehz
sehz approved these changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@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.

Great work. LGTM

@sehz
Copy link
Contributor

sehz commented Apr 7, 2023

bors r+

@sehz sehz added this to the 0.10.7 milestone Apr 7, 2023
bors bot pushed a commit that referenced this pull request Apr 7, 2023
@crajcan
SPU: Apply SmartModule Transformation For Producer (#3014)
cde0639 
#2988
@crajcan
Copy link
Contributor Author

crajcan commented Apr 7, 2023

Great work. LGTM

Thanks!

@bors
Copy link

bors bot commented Apr 7, 2023

Build failed:

@sehz
Copy link
Contributor

sehz commented Apr 7, 2023

Build failure in windows. Enable windows job and fix

@sehz
Copy link
Contributor

sehz commented Apr 7, 2023

bors r+

bors bot pushed a commit that referenced this pull request Apr 7, 2023
@crajcan
SPU: Apply SmartModule Transformation For Producer (#3014)
2476e13 
#2988
@bors
Copy link

bors bot commented Apr 7, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title SPU: Apply SmartModule Transformation For Producer [Merged by Bors] - SPU: Apply SmartModule Transformation For Producer Apr 7, 2023
@bors bors bot closed this Apr 7, 2023
@morenol morenol mentioned this pull request Apr 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morenol morenol morenol left review comments

@galibey galibey galibey requested changes

@sehz sehz sehz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10.7
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@crajcan @morenol @sehz @galibey