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: Rust V2 #219

Merged
merged 40 commits into from
Jul 1, 2024
Merged

feat: Rust V2 #219

merged 40 commits into from
Jul 1, 2024

Conversation

nplasterer
Copy link
Contributor

@nplasterer nplasterer commented Apr 9, 2024

Part of: #223
We've been seeing some android GRPC issues and also discrepancies between how Android and iOS work. Lets make them more inline with each other by both using the V2 Rust functions.

@nplasterer nplasterer self-assigned this Apr 9, 2024
@nplasterer nplasterer marked this pull request as ready for review April 10, 2024 16:02
@nplasterer nplasterer requested a review from a team as a code owner April 10, 2024 16:02

override suspend fun subscribe(topics: List<String>): Flow<Envelope> = flow {
try {
val subscription = rustV2Client.subscribe(FfiV2SubscribeRequest(topics))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason Rust hangs in the tests on these subscribe methods. I had to rework several of the tests to get this to show correctly.

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 toyed with the idea of spinning up a Android GRPC channel just for the subscribe stuff since it feels better than this rust subscribe work. But thought it would be better to just remove the dependency on android grpc entirely. Curious others thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a little scary. Have you been able to test this with the example app to see if it's the tests or the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya I ran into this with Groups as well. Everything seems to work when not using the turbine coroutine test methods. But it does make me desire a beta branch for changes like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes the example app works with streaming.

Copy link
Contributor

@neekolas neekolas left a comment

Choose a reason for hiding this comment

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

Nothing jumping out at me as being off here.

I'm not sure i understand how retries/reconnects work for streams today. That's always the trickiest part of these streaming implementations. Do we have tests around that?

@cameronvoell cameronvoell self-requested a review April 10, 2024 20:02
@nplasterer
Copy link
Contributor Author

I didn't feel comfortable with the major overhaul of streaming here. So decided to bring back the grpc stuff just for streaming. I still think this is an improvement and have confirmed streams still work as expected. I think I will merge this as an incremental step and open another PR for just the subscribe bits so that we can test them in isolation.

@nplasterer nplasterer marked this pull request as ready for review June 28, 2024 20:32
@nplasterer nplasterer requested a review from humanagent as a code owner June 28, 2024 20:32
@@ -182,6 +183,7 @@ class MainActivity : AppCompatActivity(),

private fun showError(message: String) {
val error = message.ifBlank { resources.getString(R.string.error) }
Log.e("MainActivity", message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Want this log still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya it helps with logging errors in the example. Otherwise it just says "An Error Happened" with no details.

@nplasterer nplasterer changed the title Rust V2 feat: Rust V2 Jun 28, 2024
7. Copy the jniLibs from `libxmtp/bindings_ffi/jniLibs` to `xmtp-android/library/src/main/jniLibs`
## Process for updating from a [libxmtp](https://github.com/xmtp/libxmtp) Kotlin Binding Release

1. From repo [libxmtp](https://github.com/xmtp/libxmtp) run the [kotlin release action](https://github.com/xmtp/libxmtp/actions/workflows/release-kotlin-bindings.yml) for the branch you desire
Copy link
Contributor

Choose a reason for hiding this comment

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

nice excited to try this!

environment = clientOptions.api.env,
secure = clientOptions.api.isSecure,
)
val v2Client = runBlocking {
Copy link
Contributor

@cameronvoell cameronvoell Jun 28, 2024

Choose a reason for hiding this comment

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

question - is there a reason we use runBlocking instead of making canMessage a suspend function? I know we previously had more runBlocking, but I think we pulled them out because we thought we had better performance with using suspend functions in some cases. same would apply to the other runBlocking in this class.

@cameronvoell cameronvoell self-requested a review June 28, 2024 22:47
Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

Looks good, just left a comment that I think switching some of the runBlocking for suspend functions could improve performance in some cases.

@nplasterer nplasterer merged commit 7cd738b into main Jul 1, 2024
5 of 6 checks passed
@nplasterer nplasterer deleted the np/v2-rust-grpc-fix branch July 1, 2024 05:31
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.

4 participants