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

Allow ProcessingResult to wrap record with headers #317

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

FredLive
Copy link
Contributor

Regarding the issue #313, I updated the code to be able to forward the header in case of success.

Copy link
Collaborator

@loicgreffier loicgreffier left a comment

Choose a reason for hiding this comment

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

@FredLive Here is my review

public static <K, V, V2> Record<K, ProcessingResult<V, V2>> wrapRecordSuccess(K key,
V value,
long timestamp,
Headers header) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should set headers as it is a list

@@ -106,6 +107,47 @@ public static <K, V, V2> Record<K, ProcessingResult<V, V2>> wrapRecordSuccess(K
return new Record<>(key, ProcessingResult.success(value), timestamp);
}

/**
* Wraps a key, value, timestamp and header in a Record with ProcessingResult#success(V value) as value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

headers instead of header

}

/**
* Wraps a record's value and the header with ProcessingResult.success(V value).
Copy link
Collaborator

Choose a reason for hiding this comment

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

headers instead of header

*/
public static <K, V, V2> Record<K, ProcessingResult<V, V2>> wrapRecordSuccessWithHeader(Record<K, V> message) {
return new Record<>(
message.key(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have additional tabs here

@@ -56,6 +61,65 @@ void shouldCreateWrappedProcessingResult() {
assertEquals(message.timestamp(), wrappedRecord.timestamp());
}

@Test
void shouldCreateWrappedProcessingResultWithHeader() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldCreateWrappedProcessingResultWithHeaderS

}

@Test
void shouldCreateWrappedProcessingResultWithHeaderV2() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldWrapRecordSuccessWithHeadersFromParameters would be more convenient and compliant with the previous test

Copy link
Collaborator

@loicgreffier loicgreffier left a comment

Choose a reason for hiding this comment

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

@FredLive LGTM

@loicgreffier loicgreffier changed the title New wrapRecordSuccess with header Allow ProcessingResult to wrap record with headers Feb 14, 2025
@loicgreffier loicgreffier added the feature This issue adds a new feature label Feb 14, 2025
@loicgreffier loicgreffier merged commit 59078fa into michelin:main Feb 14, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants