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.

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

feat: Message To Drop Semantics #47

Merged
merged 16 commits into from
May 30, 2024
Merged

feat: Message To Drop Semantics #47

merged 16 commits into from
May 30, 2024

Conversation

shubhamdixit863
Copy link
Contributor

This PR fixes the following issue: #12.

Map, reduce, and source transform have been modified to support drop message semantics.

How is it tested?

The scenario has been tested in a local Kubernetes cluster.

@vigith vigith requested review from vigith, yhl25 and BulkBeing May 22, 2024 14:27
@vigith vigith changed the title Message To Drop Semantics feat: Message To Drop Semantics May 22, 2024
Copy link
Member

@vigith vigith left a comment

Choose a reason for hiding this comment

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

can you update the examples to use the new semantics?

@vigith
Copy link
Member

vigith commented May 22, 2024

What about adding a func MessageToDrop() without creating the interface? It can have tags:vec![DROP.parse().unwrap()]?

src/map.rs Outdated Show resolved Hide resolved
src/map.rs Outdated
Comment on lines 108 to 128
pub fn with_keys(mut self,keys:Vec<String>)-> Self{
self.keys=keys;
self
}

pub fn with_tags(mut self,tags:Vec<String>)-> Self{
self.tags=tags;
self
}

pub fn keys(mut self) ->Vec<String>{
self.keys
}
pub fn value(mut self) ->Vec<u8>{
self.value
}

pub fn tags(mut self) ->Vec<String>{
self.tags
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering, since these are public fields, do we really need getters and setters? I like the idea of the builder pattern because these fields are optional. Should we make the fields private instead?

Copy link
Member

Choose a reason for hiding this comment

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

@BulkBeing WDYT on builder pattern? It is getting popular in Rust.

src/map.rs Outdated
Comment on lines 131 to 149
pub struct Messages{
messages:Vec<Message>
}

impl Messages{
fn message_builder() -> Self {
Messages {
messages: Vec::new(),
}
}
fn append(mut self, msg: Message) -> Self {
self.messages.push(msg);
self
}

fn items( &self) ->&Vec<Message>{
&self.messages
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can avoid this abstraction. Since users will be more comfortable using vectors directly.

@shubhamdixit863
Copy link
Contributor Author

What about adding a func MessageToDrop() without creating the interface? It can have tags:vec![DROP.parse().unwrap()]?

you mean creating just a public method without having impl part for Message struct ?

@vigith
Copy link
Member

vigith commented May 22, 2024

@shubhamdixit863 let's use builder pattern as recommended by @yhl25

@vigith
Copy link
Member

vigith commented May 23, 2024

Please run rustfmt on the changes

Signed-off-by: shubham <[email protected]>
vigith
vigith previously approved these changes May 25, 2024
@vigith
Copy link
Member

vigith commented May 25, 2024

@yhl25 @BulkBeing WDYT?

@vigith vigith dismissed their stale review May 27, 2024 16:57

i am not able to build an opinion on this, will let @yhl25 to weight in.

@yhl25
Copy link
Contributor

yhl25 commented May 27, 2024

src/map.rs Outdated
Comment on lines 92 to 105
pub fn new(value:Vec<u8>) -> Self {
Self{
value,
keys:None,
tags:None
}
}
pub fn message_to_drop(mut self) -> Self {
if self.tags.is_none(){
self.tags=Some(Vec::new());
}
self.tags.as_mut().unwrap().push(DROP.parse().unwrap());
self
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please add doc

src/map.rs Outdated
/// value: input.value,
/// tags: vec![],
/// }]
/// use numaflow::map::Message;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// use numaflow::map::Message;
/// use numaflow::map::Message;

src/map.rs Outdated
Comment on lines 123 to 129
pub fn message_to_drop(mut self) -> Self {
if self.tags.is_none(){
self.tags=Some(Vec::new());
}
self.tags.as_mut().unwrap().push(DROP.parse().unwrap());
self
}
Copy link
Member

Choose a reason for hiding this comment

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

@yhl25, @shubhamdixit863; isn't it better to have a standalone message_to_drop? what is the point of new().message_to_drop(), esp. since new takes a mandatory arg?

Copy link
Contributor

@yhl25 yhl25 May 29, 2024

Choose a reason for hiding this comment

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

message_to_drop() must be standalone!

shubhamdixit863 and others added 3 commits May 30, 2024 12:53
Copy link
Contributor

@yhl25 yhl25 left a comment

Choose a reason for hiding this comment

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

LGTM!

@yhl25 yhl25 merged commit 293ccf6 into numaproj:main May 30, 2024
2 checks passed
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.

3 participants