Added header values #51
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: shubham <[email protected]>
Signed-off-by: shubham <[email protected]>
Signed-off-by: shubham <[email protected]>
yhl25
reviewed
May 28, 2024
Signed-off-by: shubham <[email protected]>
Signed-off-by: shubham <[email protected]>
yhl25
reviewed
May 29, 2024
examples/simple-source/src/main.rs
Outdated
Comment on lines
55
to
58
| let offset = self.read_idx.load(Ordering::Relaxed); | ||
|
|
||
| let mut headers=HashMap::new(); | ||
| headers.insert(String::from("key"),String::from("key")); | ||
| // send the message to the transmitter |
There was a problem hiding this comment.
use some UUID instead of (key, key)
examples/simple-source/src/main.rs
Outdated
Comment on lines
66
to
69
| event_time: chrono::offset::Utc::now(), | ||
| keys: vec![], | ||
| headers:headers.clone() | ||
| }) |
There was a problem hiding this comment.
Yeah cloning is not good ,but sadly due to ownership rules i dnt have any other option except cloning or using Arc ,I have used Arc as of now its thread safe too @yhl25
There was a problem hiding this comment.
why can't we just give the ownership? are we using the headers anywhere else?
There was a problem hiding this comment.
the problem is every for loop iteration will be new scope ,if there was only one iteration it would have worked. @yhl25
Signed-off-by: shubham <[email protected]>
Signed-off-by: shubham <[email protected]>
vigith
reviewed
May 29, 2024
examples/simple-source/src/main.rs
Outdated
| @@ -52,7 +54,9 @@ pub(crate) mod simple_source { | |||
| self.read_idx | |||
| .store(self.read_idx.load(Ordering::Relaxed) + 1, Ordering::Relaxed); | |||
| let offset = self.read_idx.load(Ordering::Relaxed); | |||
|
|
|||
| let mut headers=HashMap::new(); | |||
| headers.insert(String::from( Uuid::new_v4()),String::from( Uuid::new_v4())); | |||
There was a problem hiding this comment.
why is the both key and value, UUID?
src/source.rs
Outdated
| @@ -320,6 +325,9 @@ mod tests { | |||
| async fn read(&self, request: SourceReadRequest, transmitter: Sender<Message>) { | |||
| let event_time = Utc::now(); | |||
| let mut message_offsets = Vec::with_capacity(request.count); | |||
| let mut headers=HashMap::new(); | |||
| headers.insert(String::from( Uuid::new_v4()),String::from( Uuid::new_v4())); | |||
src/source.rs
Outdated
| @@ -332,6 +340,7 @@ mod tests { | |||
| partition_id: 0, | |||
| }, | |||
| keys: vec![], | |||
| headers:Arc::clone(&shared_headers), // Cloning the Arc, not the HashMap, | |||
There was a problem hiding this comment.
Suggested change
| headers:Arc::clone(&shared_headers), // Cloning the Arc, not the HashMap, | |
| headers:Arc::clone(&shared_headers), |
no need of that comment since we can clearly see it is Arc::clone
Signed-off-by: shubham <[email protected]>
Signed-off-by: shubham <[email protected]>
Signed-off-by: shubham <[email protected]>
yhl25
reviewed
May 30, 2024
examples/simple-source/src/main.rs
Outdated
Comment on lines
57
to
60
| let mut headers = HashMap::new(); | ||
| let header_key=String::from(Uuid::new_v4()); | ||
| let header_value= String::from("numaflow"); | ||
| headers.insert(header_key, header_value); |
There was a problem hiding this comment.
Suggested change
| let mut headers = HashMap::new(); | |
| let header_key=String::from(Uuid::new_v4()); | |
| let header_value= String::from("numaflow"); | |
| headers.insert(header_key, header_value); | |
| let mut headers = HashMap::new(); | |
| let header_key=String::from("x-txn-id"); | |
| let header_value= String::from(Uuid::new_v4()); | |
| headers.insert(header_key, header_value); |
src/source.rs
Outdated
Comment on lines
330
to
333
| let mut headers = HashMap::new(); | ||
| let header_key=String::from(Uuid::new_v4()); | ||
| let header_value = String::from("numaflow"); | ||
| headers.insert(header_key, header_value); |
There was a problem hiding this comment.
Suggested change
| let mut headers = HashMap::new(); | |
| let header_key=String::from(Uuid::new_v4()); | |
| let header_value = String::from("numaflow"); | |
| headers.insert(header_key, header_value); | |
| let mut headers = HashMap::new(); | |
| let header_key=String::from("x-txn-id"); | |
| let header_value = String::from(Uuid::new_v4()); | |
| headers.insert(header_key, header_value); |
also move the headers inside the for loop, ideally each message will have different txn-id
Signed-off-by: shubham <[email protected]>
vigith
requested changes
May 30, 2024
Signed-off-by: shubham <[email protected]>
yhl25
approved these changes
Jun 4, 2024
vigith
approved these changes
Jun 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pr Addresses this issue #48
map ,reduce and sourcetransform proto files have been modified to include header in incoming request.
How it is tested
Tested in local cluster