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

mobile: Remove jni_log #32958

Merged
merged 2 commits into from
Mar 18, 2024
Merged

mobile: Remove jni_log #32958

merged 2 commits into from
Mar 18, 2024

Conversation

fredyw
Copy link
Member

@fredyw fredyw commented Mar 18, 2024

The jni_log_fmt and jni_log functions have empty implementations and their uses are not very useful other than cluttering the code.

Risk Level: low
Testing: CI
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #32958 was opened by fredyw.

see: more, trace.

@fredyw
Copy link
Member Author

fredyw commented Mar 18, 2024

/retest

Signed-off-by: Fredy Wijaya <[email protected]>
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw fredyw marked this pull request as ready for review March 18, 2024 19:22
@fredyw
Copy link
Member Author

fredyw commented Mar 18, 2024

/assign @abeyad

@abeyad
Copy link
Contributor

abeyad commented Mar 18, 2024

If you configure the JNI log, would it not write the output to standard logs such as when running the code in tests?

@fredyw
Copy link
Member Author

fredyw commented Mar 18, 2024

If you configure the JNI log, would it not write the output to standard logs such as when running the code in tests?

There's currently no way of configuring the JNI log other than manually updating the jni_log implementation. That's why in a8a60f6, the implementation was stubbed out when running on Android. For running the tests on Linux, we still have to do a manual std::{cout|cerr}, so in practice those JNI logs are not very worthwhile and are pretty spammy (I also haven't found them to be useful).

Copy link
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

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

thanks for the cleanup!

@abeyad abeyad merged commit 6197741 into envoyproxy:main Mar 18, 2024
35 checks passed
@fredyw fredyw deleted the remove_jni_log branch March 18, 2024 20:22
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.

2 participants