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

use the jitsi-utils logger #186

Merged
merged 9 commits into from Sep 19, 2019

Conversation

@bbaldino
Copy link
Member

commented Sep 17, 2019

No description provided.

@ibauersachs

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

Why can't we use slf4j, especially when touching a lot of logging anyway?

@bbaldino

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

Why can't we use slf4j, especially when touching a lot of logging anyway?

There are 2 main questions here: one is the the log context and the other is the enforcement of the minimum log level from the top down. The log context stuff slf4j does have support for and making it work with java.util.logger it looks like it should be doable? But I haven't taken a look at it. The other feature is kinda unique and I'm not sure if slf4j supports it (basically we need to enforce that a parent logger and all of its children don't log messages below a certain level, even if they are configured to).

If you've got any insight on those I'd be interested to hear it. I'll admit, though, that the main reason I didn't change to slf4j or dig too deeply into whether or not it supports these things is that, as big as these changes may seem, they're still entirely superficial and I didn't want to (lazy?) get into the work of validating a change of logging framework.

@bbaldino bbaldino closed this Sep 18, 2019
@bbaldino bbaldino reopened this Sep 18, 2019
@bbaldino

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2019

jenkins, test this please

@bbaldino

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2019

so i think it may be impossible to get the tests passing here. jicofo fails to build because it relies on the bridge, but we can't update the bridge dependency because we don't have one until we have an ice4j version...which we won't get until this PR merges :)

@@ -91,12 +90,10 @@ public LocalCandidate(TransportAddress transportAddress,
CandidateType type,
CandidateExtendedType extendedType,
LocalCandidate relatedCandidate)

This comment has been minimized.

Copy link
@virtuacoplenny

virtuacoplenny Sep 18, 2019

nit: extra line break?

@damencho

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Hum, all with same branch should use those branches as dependencies, shouldn't that work?

@damencho

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

The problem is that the PR testing scripts does not take care of jitsi-utils at all.

@damencho

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Seems to be fixed now the jitsi-utils situation, but for some reason in jicofo logs I see it uses /home/jenkins/.m2/repository/org/jitsi/ice4j/2.0.0-SNAPSHOT/ice4j-2.0.0-SNAPSHOT.jar still not sure why.

@damencho

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Created 2 dummy PRs in jicoco and jitsi in order to update jicofo dependency to ice4j when testing using the videobridge...

@bbaldino bbaldino merged commit e854e4e into jitsi:master Sep 19, 2019
1 check passed
1 check passed
default 414 tests run, 286 skipped, 0 failed.
Details
@bbaldino bbaldino deleted the bbaldino:jitsi_utils_logger branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.