search

apache / incubator-wayang

Apache Wayang(incubating) is the first cross-platform data processing system.
https://wayang.incubator.apache.org/
Apache License 2.0
189 stars 82 forks source link

Set logLevel on WayangContext to allow doing it without conf file #453

Closed juripetersen closed 3 months ago

juripetersen commented 5 months ago

This proposes a functional interface to change the Loggers level with the WayangContext:

context.setLogLevel(Level.ERROR);

The change provides better ergonomics and prevents having to fiddle with configuration files, when Context objects etc. can be created during plan building. Furthermore, this aligns with the API presented in other Apache projects like Spark

kbeedkar commented 5 months ago

Can you check why some unit tests are failing?

juripetersen commented 5 months ago

Can you check why some unit tests are failing?

I can't replicate the error locally. It looks like building the assembly fails due to local/cache problems? Maybe a rerun of the action solves it?

juripetersen commented 5 months ago

@kbeedkar it seems like the build can't find wayang-api-json in https://repo.maven.apache.org/maven2/org/apache/wayang/. This seems to be correct, as that module is not to be found there. Do we have to publish a release for that module to be in maven-central?

2pk03 commented 5 months ago

@juripetersen - yes I think so.

zkaoudi commented 5 months ago

But the change does not concern the missing module, so that is weird. Also the module is locally located.

juripetersen commented 5 months ago

@zkaoudi @kbeedkar I can reproduce this by running ./mvnw clean verify -B -Dmaven.test.skip=false -Dwayang.configuration=file:$(pwd)/tools/test/config/wayang.properties on a fresh setup. The error occurs because some modules are not on mvn central as they are not in a release yet. I think this is sensible and the build should be constructed from the current state of the repo and not from central. If I run mvn clean install before, the local modules are built and then used for the verification step. This resolves the error.

Is including install as a step before verification in our action a solution, or does this introduce something I am unaware of?

zkaoudi commented 5 months ago

@juripetersen makes totally sense. @2pk03 do you see any issue with that?