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

expose the connection state to consumer and producer interface #239 #240

Merged
merged 2 commits into from
Nov 18, 2023
Merged

expose the connection state to consumer and producer interface #239 #240

merged 2 commits into from
Nov 18, 2023

Conversation

baumerik
Copy link
Contributor

Hi @Lanayx,

here is my attempt for exposing the IsConnected.
For the MultiTopicsConsumerImpl and the PartitionedProducerImpl I've assumed that it will be return true just when all Topics are connected and ready.
I've tested it localy it with the MultiTopicsConsumerImpl and the property switched es expected.
please give me feedback if it fits to your expectation.

Thanks
Erik

@Lanayx
Copy link
Collaborator

Lanayx commented Nov 17, 2023 •
edited
Loading

Thanks for the PR, it looks good! Can you please also add it to the Reader interface and implementation, just like in Java code

Lanayx
Lanayx requested changes Nov 17, 2023
View reviewed changes
src/Pulsar.Client/Internal/PartitionedProducerImpl.fs Outdated
Comment on lines 204 to 207
producers
|> Seq.map (fun producer -> producer.IsConnected)
|> Seq.contains(false)
|> not
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can replace it with
Seq.forall _.IsConnected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this syntax does not work but I changed it to forall

src/Pulsar.Client/Internal/MultiTopicsConsumerImpl.fs Outdated
Comment on lines 815 to 817
|> Seq.map (fun (KeyValue(_, (consumer, _))) -> consumer.IsConnected)
|> Seq.contains(false)
|> not
Copy link
Collaborator

@Lanayx Lanayx Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think you can replace it with
Seq.forall (fun (KeyValue(_, (consumer, _))) -> consumer.IsConnected)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks much better 😃

src/Pulsar.Client/Internal/ConsumerImpl.fs Outdated
Comment on lines 1669 to 1671
match connectionHandler.ConnectionState with
| Ready _ -> true
| _ -> false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please ident | back to be right under match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/Pulsar.Client/Internal/ProducerImpl.fs Outdated
Comment on lines 909 to 911
match connectionHandler.ConnectionState with
| Ready _ -> true
| _ -> false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please ident | back to be right under match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@baumerik
Copy link
Contributor Author

baumerik commented Nov 18, 2023 •
edited
Loading

Thanks for the PR, it looks good!
Thanks for my first Feedback to F# 😄

Can you please also add it to the Reader interface and implementation, just like in Java code
sorry, this i haven't see in the first time. It is now added.

@Lanayx Lanayx merged commit 6834cff into fsprojects:2.x Nov 18, 2023
2 checks passed
@Lanayx
Copy link
Collaborator

Lanayx commented Nov 18, 2023

Merged! Do you want to do this for 3.0 branch as well? 2.x will not receive performance updates and refactoring in future

@Lanayx
Copy link
Collaborator

Lanayx commented Nov 18, 2023

https://www.nuget.org/packages/Pulsar.Client/2.15.0 - this was published

@baumerik baumerik deleted the expose_isconnected branch November 19, 2023 18:51
Lanayx added a commit that referenced this pull request Nov 19, 2023
@baumerik @Lanayx
Merge for IsConnected from v2 (#241)
5b22421 
* expose the connection state to consumer and producer interface #239 (#240)

* expose the connection state to consumer and producer interface #239

* fix issues from code review and add reader interface #239

* 2.15.0 release

---------

Co-authored-by: Vladimir Shchur <odindafna2006@rambler.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lanayx Lanayx Lanayx requested changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@baumerik @Lanayx