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] - feat: inject timestamp to record in SmartModule context #3389

Closed
wants to merge 8 commits into from
Closed

[Merged by Bors] - feat: inject timestamp to record in SmartModule context #3389

wants to merge 8 commits into from

Conversation

EstebanBorai
Copy link
Contributor

@EstebanBorai EstebanBorai commented Jul 11, 2023 •
edited

Introduces the base_timestamp and a new Record wrapper for SmartModules called
SmartModuleRecord which holds the base_timestamp and base_offset.

Along with changes to introduce the new fields, some API changes are also needed to
move into an extensible API, the fluvio_smartmodule::Record will be migrated to fluvio_smartmodule::SmartModuleRecord.

Existing SmartModules will continue to work, but a compile time warning will be shown:

warning: use of deprecated method `fluvio_smartmodule::dataplane::smartmodule::SmartModuleInput::into_legacy_records`: use SmartModuleRecord instead

@EstebanBorai EstebanBorai linked an issue Jul 11, 2023 that may be closed by this pull request
Inject base timestamp to SmartEngine context #3388
Closed
@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch 2 times, most recently from 61d854f to e917651 Compare July 13, 2023 20:10
@EstebanBorai
Copy link
Contributor Author

Looks like Im not encoding/decoding entire bits...

 left: `[
    1, 0, 4, 116, 95, 105, 100, 255, 255, 0, 0, 3, 232, 0, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 0, 4, 222,
    173, 190, 239, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 20, 0, 0, 0, 0
]`,
 right: `[
    1, 0, 4, 116, 95, 105, 100, 255, 255, 0, 0, 3, 232, 0, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 0, 4, 222,
    173, 190, 239, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1
]`', crates/fluvio-spu-schema/src/produce/request.rs:506:9

@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch from ca4f5a8 to 815652e Compare July 14, 2023 16:43
@sehz
Copy link
Contributor

sehz commented Jul 14, 2023

we may need to additional timestamp to compute age of the record

@EstebanBorai
Copy link
Contributor Author

EstebanBorai commented Jul 14, 2023 •
edited

we may need to additional timestamp to compute age of the record

Roger that!

morenol
morenol reviewed Jul 14, 2023
View reviewed changes
Copy link
Contributor

@morenol morenol left a comment

Choose a reason for hiding this comment

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

I think that I approved this PR by error. Commenting to remove my approve review

@morenol morenol self-requested a review July 14, 2023 17:15
@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch 4 times, most recently from 6834a09 to 0aa31e1 Compare July 15, 2023 17:37
@EstebanBorai EstebanBorai changed the title feat: batch timestamp for SmartModuleInput feat: inject timestamp to SmartModuleInput Jul 15, 2023
@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch 5 times, most recently from 8b96bae to c35945d Compare July 17, 2023 17:55
@EstebanBorai EstebanBorai changed the title feat: inject timestamp to SmartModuleInput feat: inject timestamp to record in SmartModule context Jul 17, 2023
@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch 9 times, most recently from 199ed62 to 978870f Compare July 18, 2023 22:42
@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch 4 times, most recently from 291f504 to c27435d Compare July 25, 2023 04:34
@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch from c27435d to 07f3450 Compare July 25, 2023 04:55
@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch from 1990028 to c560775 Compare July 25, 2023 14:44
@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch from c560775 to e0041f3 Compare July 25, 2023 14:50
galibey
galibey approved these changes Jul 25, 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.

LGTM, just need to make tests green

@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch 3 times, most recently from f677d3c to e870f61 Compare July 26, 2023 17:29
@EstebanBorai EstebanBorai force-pushed the 3388-inject-base-timestamp-to-smartengine-context branch from e870f61 to fc1083d Compare July 26, 2023 17:45
@digikata digikata added this to the 0.10.14 milestone Jul 26, 2023
@EstebanBorai EstebanBorai requested a review from sehz July 26, 2023 19:03
sehz
sehz approved these changes Jul 26, 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.

LGTM

@EstebanBorai
Copy link
Contributor Author

bors r+

bors bot pushed a commit that referenced this pull request Jul 26, 2023
@EstebanBorai
feat: inject timestamp to record in SmartModule context (#3389)
57d72a6 
Introduces the `base_timestamp` and a new `Record` wrapper for SmartModules called
`SmartModuleRecord` which holds the `base_timestamp` and `base_offset`.

Along with changes to introduce the new fields, some API changes are also needed to
move into an extensible API, the `fluvio_smartmodule::Record` will be migrated to `fluvio_smartmodule::SmartModuleRecord`.

Existing SmartModules will continue to work, but a compile time warning will be shown:

```
warning: use of deprecated method `fluvio_smartmodule::dataplane::smartmodule::SmartModuleInput::into_legacy_records`: use SmartModuleRecord instead
```
@bors
Copy link

bors bot commented Jul 26, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title feat: inject timestamp to record in SmartModule context [Merged by Bors] - feat: inject timestamp to record in SmartModule context Jul 26, 2023
@bors bors bot closed this Jul 26, 2023
@EstebanBorai EstebanBorai mentioned this pull request Jul 26, 2023
Closed
@EstebanBorai EstebanBorai mentioned this pull request Aug 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morenol morenol morenol approved these changes

@sehz sehz sehz approved these changes

@galibey galibey galibey approved these changes

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

Successfully merging this pull request may close these issues.

Inject base timestamp to SmartEngine context
5 participants
@EstebanBorai @sehz @morenol @galibey @digikata