Skip to content

Conversation

@johnpoth
Copy link
Member

@johnpoth johnpoth commented Nov 15, 2019

@tdiesler can you confirm this resolves the issue your raised and makes camel-infinispan compatible ?

cc @oscerd

Thanks !

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

cc @tdiesler

@tdiesler
Copy link
Contributor

merci. I'll take a look ...

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

I'm merging this one

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

Merged. Thanks.

@oscerd oscerd closed this Nov 18, 2019
@tdiesler
Copy link
Contributor

This branch does not compile

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project camel-spring-ws: Compilation failure
[ERROR] /Users/tdiesler/git/camel/components/camel-spring-ws/src/main/java/org/apache/camel/component/spring/ws/filter/impl/HeaderTransformationMessageFilter.java:[171,32] error: cannot find symbol
[ERROR]   symbol:   variable SAXON_TRANSFORMER_FACTORY_CLASS_NAME
[ERROR]   location: class XsltEndpoint

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

Master branch is fine. Just build locally.

https://builds.apache.org/view/C/view/Apache%20Camel/job/Camel/job/master/1682/#showFailuresLink

This is the last build.

@davsclaus
Copy link
Contributor

This has been fixed on latest master. As the PR has been merged, then try with master branch

@tdiesler
Copy link
Contributor

tdiesler commented Nov 18, 2019

More often than not, I see units of work spread out over many commits and (as it seems) also being eagerly committed without basic verification (i.e. does it even compile). This more often than not violates the good practice of "master is always good". Why this rush? So far, I have not heard a good argument of why even basic verification of any given change set isn't necessary.

As you know, I'm a proponent of an almost opposite approach i.e. comprehensive verification must happen before any change set can be committed to a public branch. Every change set must be complete and ideally be just one (squashed) commit. In other words, you share when you're ready and not before. You only share what is guaranteed to be good. The CI env is the only entity that commits because it is the only entity that can reliably provide such a guarantee.

Many well managed projects work like this and I find it very comforting knowing that I can checkout any given public branch at any time and that it is always guaranteed to be good.

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

Using this approach makes the development slower. A full build requires 7 hours at least and this is not feasible for each PRs. So far we didn't hear complaints about the workflow and I think what we release it's good enough. There is no rush. I personally check each PR locally and run full build each time. But this is not a problem. We can fix it later. I disagree with the CI should be the only entity that commit. It doesn't make sense. The actual camel workflow works fine.

@tdiesler
Copy link
Contributor

tdiesler commented Nov 18, 2019

A workflow that produces public branches that do not even compile is "not fine". The CI needs to provide a good level of verification not a perfect level. A build including a good level of verification should not take longer than 30min (perhaps) and I dare to say that stuff is unlikely to be good and complete at the same rate. It does not have to be a full build. In other words the CI can likely easily keep up with all the good work that is happening as long as "good work" also means "completed unit of work"

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

There is no way of having a full build in 30 mins

@tdiesler
Copy link
Contributor

tdiesler commented Nov 18, 2019

"It does not have to be a full build" - a good level of verification is sufficient. In WildFly-Camel we have three levels of verification ...

  • Smoke: verifies the critical core functionality (2:30 min)
  • Basic: verifies basic functionality ca. 75% of all stuff. Done for every PR (25 min)
  • All: runs every now and then and detects broken stuff not covered by basic

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

We already experimented with partial builds, bazel etc. Nothing work. Provide a solution, otherwise this discussion will still be useless, as always.

@tdiesler
Copy link
Contributor

I'm happy to provide that change to your build process as long as there is a commitment from the key people to: Yes, we want "master is always good"

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

I don't think is critical, but a good enhancement. Nevertheless I believe is not feasible.

@tdiesler
Copy link
Contributor

tdiesler commented Nov 18, 2019

So, shall we have a deal ... if I can change the build such that it can keep up with the rate of high quality PRs (i.e. stuff ready for sharing) you would agree to ... No more spread out commits. No more manual commits.

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

An agreement require all the PMC or at least a subset of active committers to agree. I didn't say there won't be manual commits. I'm just saying if you're able to create a pr job that makes a build in less than 7 hours, we'll use it

@tdiesler
Copy link
Contributor

@johnpoth Your change works fine - thanks a million - good work.

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

In 30 minutes, maybe you'll build the first 50 modules. Maybe.

@tdiesler
Copy link
Contributor

tdiesler commented Nov 18, 2019

I currently build all camel modules with -DskipTests in ...

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  30:26 min
[INFO] Finished at: 2019-11-18T10:39:47+01:00
[INFO] ------------------------------------------------------------------------

This build time I can reduce considerably by skipping springboot stuff and possibly other optional stuff - if you hold my hand a little. Then I can add to the reduced build time basic testing that covers the scope of verification we like to have for every PR. Ideally, I'd end up with a build time that includes a "good level" of verification in ~ 30min

@davsclaus
Copy link
Contributor

The ASF runs with a trust for committers whom can commit or do anything. If you have comments for that work, then post on dev mailing list and state your reason. And a reason can not be of personal opinion/taste etc, but should be technical. Like this commit made Camel 25% slower on startup etc. Or that it opened up a door for security exploits etc.

You may have your own habit of 1 commit per change, and only commit if some CI server has run to completion and passed 100% and other checks etc.

But at the end of the day, the ASF policy is very open, and Camel is run as-is. Its a very large code-base and very open.

Work to speedup the CI build is welcome, and some attempts in the past with parallel builders and others have failed, or jenkins/maven are not clever enough etc.

@gnodet made the re-build faster, and other faster build improvements, if you do a mvn install -P fastinstall then it can run in about 6 mins.

The spring-boot starters is a candidate to be moved to its own sub-project, ala we do with camel-quarkus etc. But its a big effort and can likely happen later in 3.x.

@ppalaga
Copy link
Contributor

ppalaga commented Nov 18, 2019

You may have your own habit of 1 commit per change, and only commit if some CI server has run to completion and passed 100% and other checks etc.

Claus, in all respect to your position in this project, those are good practices accepted throughout the industry rather than @tdiesler's or mine personal choices. The quality should matter to all of us.

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

There is always space for improvements. There is no space for polemizing instead.

@oscerd
Copy link
Contributor

oscerd commented Nov 18, 2019

And by the way doing an incremental build approach is not feasible, because the interconnection between modules is not linear. You may break karaf features, sb itest, test in other modules, examples. You'll need a full rebuild to be 100% sure.

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.

5 participants