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

Revert "Add ASF license to optional XML files and last gradle file." #3705

Merged
merged 1 commit into from May 25, 2018

Conversation

cbickel
Copy link
Contributor

@cbickel cbickel commented May 25, 2018

Reverts #3702

The PR above breaks the logging of all our components. After this, all our logs do not appear anymore.
This affects all our components and gatling tests.

Therefore this PR reverts this commit.

@rabbah
Copy link
Member

rabbah commented May 25, 2018

@cbickel can you explain the reason for reverting - ie how are the comment/license headers an issue? Thanks.

@cbickel
Copy link
Contributor Author

cbickel commented May 25, 2018

@rabbah Sorry, I updated the description.

@codecov-io
Copy link

Codecov Report

Merging #3705 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3705   +/-   ##
======================================
  Coverage    74.5%   74.5%           
======================================
  Files         126     126           
  Lines        5994    5994           
  Branches      392     392           
======================================
  Hits         4466    4466           
  Misses       1528    1528

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cc2227...582d576. Read the comment docs.

Copy link
Contributor

@jeremiaswerner jeremiaswerner left a comment

Choose a reason for hiding this comment

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

LGTM

@jeremiaswerner jeremiaswerner merged commit 441fc71 into master May 25, 2018
@csantanapr
Copy link
Member

That’s odd how come adding comments at the top of an xml brakes functionality?

@csantanapr
Copy link
Member

What’s the root cause? How do we fix instead of doing a drastic revert with no other options being proposed?

@csantanapr
Copy link
Member

For the most part all files should have an ASF header, if someone can proposed how can we get the xml files with the header or state why logging brakes and there is no workaround and we are forced to excempt the files with explanation from having the ASF license header

@cbickel cbickel deleted the revert-3702-asf-xml branch May 25, 2018 12:01
@sven-lange-last
Copy link
Member

This is the exception occurring with the XML comment:

Failed to auto configure default logger context
Reported exception:
ch.qos.logback.core.joran.spi.JoranException: Problem parsing XML document. See previously reported errors.
    at ch.qos.logback.core.joran.event.SaxEventRecorder.recordEvents(SaxEventRecorder.java:65)
    at ch.qos.logback.core.joran.GenericConfigurator.doConfigure(GenericConfigurator.java:151)
    at ch.qos.logback.core.joran.GenericConfigurator.doConfigure(GenericConfigurator.java:110)
    at ch.qos.logback.core.joran.GenericConfigurator.doConfigure(GenericConfigurator.java:53)
    at ch.qos.logback.classic.util.ContextInitializer.configureByResource(ContextInitializer.java:75)
    at ch.qos.logback.classic.util.ContextInitializer.autoConfig(ContextInitializer.java:150)
    at org.slf4j.impl.StaticLoggerBinder.init(StaticLoggerBinder.java:84)
    at org.slf4j.impl.StaticLoggerBinder.<clinit>(StaticLoggerBinder.java:55)
    at org.slf4j.LoggerFactory.bind(LoggerFactory.java:150)
    at org.slf4j.LoggerFactory.performInitialization(LoggerFactory.java:124)
    at org.slf4j.LoggerFactory.getILoggerFactory(LoggerFactory.java:412)
    at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:357)
    at kamon.util.logger.LazyLogger$.apply(LazyLogger.scala:46)
    at kamon.util.logger.LazyLogger$.apply(LazyLogger.scala:47)
    at kamon.Kamon$Instance.<init>(Kamon.scala:50)
    at kamon.Kamon$.<init>(Kamon.scala:30)
    at kamon.Kamon$.<clinit>(Kamon.scala)
    at whisk.core.controller.Controller$.main(Controller.scala:188)
    at whisk.core.controller.Controller.main(Controller.scala)
Caused by: org.xml.sax.SAXParseException; systemId: jar:file:/controller/lib/openwhisk-common.jar!/logback.xml; lineNumber: 5; columnNumber: 6; The processing instruction target matching "[xX][mM][lL]" is not allowed.
    at com.sun.org.apache.xerces.internal.parsers.AbstractSAXParser.parse(AbstractSAXParser.java:1239)
    at com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl$JAXPSAXParser.parse(SAXParserImpl.java:643)
    at com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl.parse(SAXParserImpl.java:327)
    at ch.qos.logback.core.joran.event.SaxEventRecorder.recordEvents(SaxEventRecorder.java:59)
    ... 18 more
00:01:20,741 |-INFO in ch.qos.logback.classic.LoggerContext[default] - Could NOT find resource [logback-test.xml]
00:01:20,741 |-INFO in ch.qos.logback.classic.LoggerContext[default] - Could NOT find resource [logback.groovy]
00:01:20,741 |-INFO in ch.qos.logback.classic.LoggerContext[default] - Found resource [logback.xml] at [jar:file:/controller/lib/openwhisk-common.jar!/logback.xml]
00:01:20,769 |-INFO in ch.qos.logback.core.joran.spi.ConfigurationWatchList@3fb1549b - URL [jar:file:/controller/lib/openwhisk-common.jar!/logback.xml] is not of type file
00:01:20,836 |-ERROR in ch.qos.logback.core.joran.event.SaxEventRecorder@ea6147e - XML_PARSING - Parsing fatal error on line 5 and column 6
00:01:20,836 |-ERROR in ch.qos.logback.core.joran.event.SaxEventRecorder@ea6147e - org.xml.sax.SAXParseException; systemId: jar:file:/controller/lib/openwhisk-common.jar!/logback.xml; lineNumber: 5; columnNumber: 6; The processing instruction target matching "[xX][mM][lL]" is not allowed.

@rabbah
Copy link
Member

rabbah commented May 25, 2018

did you try having <?xml version="1.0" encoding="UTF-8"?> as the first line and comment for license to follow --- just a guess.

@mhenke1
Copy link
Contributor

mhenke1 commented May 25, 2018

^^^That is the definitive solution
https://stackoverflow.com/a/1196485

@chetanmeh
Copy link
Member

What @rabbah mentioned should work as we use similar approach at https://github.com/apache/jackrabbit-oak/blob/trunk/oak-run/src/main/resources/logback.xml

@csantanapr
Copy link
Member

yeah it looks having the <?xml version="1.0" encoding="UTF-8"?> at the top is the solution and adjust the code scan to assume that xml ASF licenses start with this line.

@csantanapr
Copy link
Member

@mrutkows ^^

@csantanapr
Copy link
Member

csantanapr commented May 25, 2018

@cbickel @jeremiaswerner since you reverted and took the ASF license header out, What do you think of having the files include the ASF header and the files start with <?xml version="1.0" encoding="UTF-8"?>

Would you revert again ?

@csantanapr
Copy link
Member

Now that I think about it how come Travis didn't catch this problem in the PR that introduce the header?

@csantanapr
Copy link
Member

Yeah I see Travis is 💚 but I see the failure
https://travis-ci.org/apache/incubator-openwhisk/jobs/383311969#L3422

:tests:performance:gatling_tests:gatlingRun-BlockingInvokeOneActionSimulationFailed to auto configure default logger context
Reported exception:
ch.qos.logback.core.joran.spi.JoranException: Problem parsing XML document. See previously reported errors.
	at ch.qos.logback.core.joran.event.SaxEventRecorder.recordEvents(SaxEventRecorder.java:65)
	at ch.qos.logback.core.joran.GenericConfigurator.doConfigure(GenericConfigurator.java:151)

and just continue and some tests actually ran. 🤔

@csantanapr
Copy link
Member

csantanapr commented May 25, 2018

I see it looks like it only cause problems with generating the logs.
But the gradle task continued.
Maybe the tests ran, it didn't generate logs
and ended with 0 exit code

The command "OPENWHISK_HOST="172.17.0.1" MEAN_RESPONSE_TIME="1000" 
API_KEY="$(cat ansible/files/auth.guest)" EXCLUDED_KINDS="python:default,java:default,swift:default" 
./gradlew gatlingRun-LatencySimulation" exited with 0.

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.

None yet

8 participants