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

chore: add VectorSink::Sink #4846

Merged
merged 10 commits into from
Nov 6, 2020
Merged

chore: add VectorSink::Sink #4846

merged 10 commits into from
Nov 6, 2020

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Nov 2, 2020

Part of #2942 #4247

Only introduce variant and upgrade pulsar and kafka. Integration tests for kafka fail for me locally, will mark as draft for some time.

@fanatid fanatid added sink: kafka Anything `kafka` sink related sink: pulsar Anything `pulsar` sink related labels Nov 2, 2020
@fanatid fanatid added this to the 2020-10-26: Recognizer milestone Nov 2, 2020
@fanatid fanatid self-assigned this Nov 2, 2020
@fanatid fanatid marked this pull request as ready for review November 2, 2020 21:46
@fanatid fanatid requested a review from lukesteensen as a code owner November 2, 2020 21:46
@fanatid fanatid requested a review from bruceg November 2, 2020 21:46
src/sinks/pulsar.rs Outdated Show resolved Hide resolved
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
record = record.timestamp(timestamp.timestamp_millis());
}
let producer = Arc::clone(&self.producer);
self.delivery_fut.push(Box::pin(async move {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

send_result is not async and previously we returned NotReady if the queue was full. In the new Sink trait we can not return not ready, so for the case when sending blocked (queue full) I added FuturesUnordered where send_result called with 10ms delay. A number of futures limited by SEND_RESULT_LIMIT (set to 5).
I'm not completely sure that this is the best solution, maybe have 1 future with 1ms (or something like this) delay will be better.

Copy link
Member

Choose a reason for hiding this comment

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

Just to follow up on this, I think what you have here is reasonable.

If we wanted to be more precise, we could try to tie the retries to the completion of the in_flight futures instead of a delay. So when one of those is complete we try again to send data, assuming that the completed request could have opened up space in the buffer. But that could be a bit tricky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right, that sounds more correct way. I agree that this can be tricky, but should I try to add it?

Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth spending a little bit of time to see how it would work, but not a lot if it turns out to be complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in #5007

let message = encode_event(item, &self.encoding).map_err(|_| ())?;

let producer = Arc::clone(&self.producer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an internal state instead Arc<Mutex<T>>.

Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
@fanatid fanatid added the type: tech debt A code change that does not add user value. label Nov 5, 2020
Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

Looks ok on my end, but I don't know what the best approach for the semantics of this should be, particularly your questions about the librdkafka interface.

src/sinks/kafka.rs Outdated Show resolved Hide resolved
src/sinks/kafka.rs Outdated Show resolved Hide resolved
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
@fanatid fanatid merged commit c526860 into master Nov 6, 2020
@fanatid fanatid deleted the sinks-futures-0.3 branch November 6, 2020 09:54
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
Signed-off-by: Kirill Fomichev <fanatid@ya.ru>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sink: kafka Anything `kafka` sink related sink: pulsar Anything `pulsar` sink related type: tech debt A code change that does not add user value.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants