Skip to content

Inline OkHttpNetworkService #2126

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

Merged
priettt merged 1 commit into from
Apr 25, 2025
Merged

Inline OkHttpNetworkService #2126

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions ...n/src/main/java/io/embrace/android/gradle/plugin/tasks/ndk/UploadSharedObjectFilesTask.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,6 @@ abstract class UploadSharedObjectFilesTask @Inject constructor(

private lateinit var architecturesToHashedSharedObjectFilesMap: Map<String, Map<String, String>>

private val okHttpNetworkService by lazy {
OkHttpNetworkService(requestParams.get().baseUrl)
}

@TaskAction
fun onRun() {
try {
Expand Down Expand Up @@ -90,7 +86,7 @@ abstract class UploadSharedObjectFilesTask @Inject constructor(
variantData.get().name,
architecturesToHashedSharedObjectFilesMap
)

val okHttpNetworkService = OkHttpNetworkService(requestParams.get().baseUrl)
Copy link
Contributor

@bidetofevil bidetofevil Apr 25, 2025

Choose a reason for hiding this comment

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

If this is never reused, any reason why you don't just do the init in NdkUploadHandshake?

Or frankly, just extract that method out of the class and put in in this class? It was initially confusing what we are doing - initializing an object and then throwing it away, when it really is just a method call that internally needs an objet

Copy link
Contributor Author

@priettt priettt Apr 25, 2025

Choose a reason for hiding this comment

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

Mostly because I thought it looked better than inlining 😅

I can change it if you have a strong preference, but we will probably be revisiting this later

val handshakeResult = NdkUploadHandshake(okHttpNetworkService).getRequestedSymbols(params, failBuildOnUploadErrors.get())
?: error("There was an error while reporting shared object files")
return handshakeResult
Expand All @@ -100,6 +96,7 @@ abstract class UploadSharedObjectFilesTask @Inject constructor(
* Attempt to upload requested symbols per architecture
*/
private fun uploadSharedObjectFiles(requestedSharedObjectFiles: Map<String, Map<String, File>>) {
val okHttpNetworkService = OkHttpNetworkService(requestParams.get().baseUrl)
// TODO: We're spanning multiple http requests, shouldn't we batch these? Having a single request per file doesn't look optimal.
requestedSharedObjectFiles.forEach { (arch, files) ->
files.forEach { (id: String, symbolFile: File) ->
Expand Down
Loading