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

Add support for Java8 datatypes #13

Closed
wants to merge 1 commit into from
Closed

Conversation

langfr
Copy link
Contributor

@langfr langfr commented May 4, 2017

No description provided.

@JoHeinem JoHeinem self-assigned this May 9, 2017
@JoHeinem
Copy link

JoHeinem commented May 9, 2017

Hey @langfr ,

Right now we are planning to release the new minor Camunda version 7.7 by the end of the month. Upgrading to the latest Jackson version and adding support for Java8 datatypes adds a certain risk to a smooth release as this might cause unexpected problems. Therefore, we would like to come back to you in 3 weeks and reconsider your pull requests.

Is that fine for you or do you have a desperate need to have your desired functionality in Camunda 7.7?

Best,
Johannes

@langfr
Copy link
Contributor Author

langfr commented May 9, 2017

Hi @JoHeinem,
no problem if you postpone this to Camunda 7.8.
I anyways maintain a camunda fork because of another feature added (IBM Informix Database support).
So this currently uses the camunda-spin snapshot including this PR here.
The upgrade of the jackson version is not evident, I just saw the existing Jira for doing this in cmaunda-spin and created the PR. New Jira and PR for camunda-rest to use the same version.
Regards, Frank

@JoHeinem
Copy link

Hi @langfr,

As we are done with the release, I'll have a look at your pull requests within the next couple of days. Thanks for the patience :-)

Best,
Johannes

@JoHeinem
Copy link

Hey @langfr ,

We have merged your PR.

Best,
Johannes

@JoHeinem JoHeinem closed this Jun 22, 2017
@JoHeinem
Copy link

JoHeinem commented Jun 22, 2017

Hey @langfr,

I'm sorry for the confusion, but I had to revert your PR's, because a lot of integration tests were failing because of them. I couldn't really figure out, what the exact problem was, but we weren't able to deploy ear/war files on wildfly/jboss anymore. That is the error that often occurred: https://pastebin.com/sGvaYfZG

Also it was not fully possible to use Spin as a plugin anymore. The following error occurred in these cases: https://pastebin.com/10apvuKC

Because of this, I can't unfortunately accept your pull requests for now. Do you intend to provide another pull request?

Best,
Johannes

@JoHeinem JoHeinem reopened this Jun 22, 2017
@langfr
Copy link
Contributor Author

langfr commented Jun 22, 2017

Hey Johannes,

seems you're still running builds and tests using JDK 6. :-(
Jackson-Databind needs JDK 7 starting with release 2.7.
This is causing the
java.lang.UnsupportedClassVersionError: spinjar/com/fasterxml/jackson/databind/JsonNode : Unsupported major.minor version 51.0.
So the maximum version that can be used for camunda-* is 2.6.7.
Merging this single PR only should not break anything.
Afterwards I can create new PRs for the jackson upgrade from 2.6.0/2.6.3 to 2.6.7 for both, camunda-spin and camunda-bpm-platform.

Best regards, Frank

@JoHeinem
Copy link

Hi Frank,

the problem there is that we support JDK 6 onward as Java Runtime environment. So if we would change that, a lot of customers might not use the platform anymore. Therefore, we also test against JDK 6. I know, it's a hassle (also for us as developers), but I hope you understand that we can't change this.

Merging this single PR only should not break anything.

Does it make sense for you to just have this PR? I always saw all your PR's as a whole and tried to add them all at once to get your desired functionality. Therefore, I think it makes sense, if you create new PR's for the jackson upgrades in camunda-spin and camunda-bpm-platform and then I try to merge this one again. But I'm open to suggestions :-)

Best,
Johannes

@langfr
Copy link
Contributor Author

langfr commented Jun 23, 2017

Hi Johannes,
the version upgrade of jackson in camunda-spin and camunda-bpm-platform is not mandatory, it can be done independently of adding the jackson datatype JSR310 module to camunda-spin and camunda-bpm-platform.
With this applied, I could use your snapshot builds again and can throw away my local build of camunda-spin.

So after merging this PR here the next step would be to merge https://github.com/camunda/camunda-bpm-platform/pull/270/files (or a new one) to camunda-bpm-platform.

Will merging this PR on cmaunda-spin trigger a new version 1.3.2 of cmaunda-spin?
Or do we have to use camunda.spin.verison 1.4.0-SNAPSHOT in master of camunda-bpm-platform/bom/pom.xml?

Regards, Frank

@JoHeinem
Copy link

JoHeinem commented Jun 26, 2017

Hi @langfr,

Alright, then I'll try to merge those two PR's to Camunda again and report back the result here.

As long as we don't have a new minor Camunda release (so until fall), we're gonna have a minor snapshot version, meaning 1.4.0-SNAPSHOT. However, as soon as we release Camunda, we're gonna release all secondary projects as well. So after fall the new version would be 1.4.0. Is that a problem? Otherwise we can also release it earlier.

Best,
Johannes

@JoHeinem
Copy link

Hi Frank,

I merged the two PR's (under reserve) and so far everything looks good :-)
However, I'm on vacation until next Monday. If no problems occur until then, we can accept you changes for sure. Hope that's fine for you.

Best,
Johannes

@langfr
Copy link
Contributor Author

langfr commented Jun 27, 2017

👍

@romansmirnov
Copy link
Member

Hi @JoHeinem, Hi @langfr ,

Unfortunately, I had to revert the changes related to this pull request. Due to this changes the weblogic integration test cases are failing.

We are going to build an alpha release at the end of the week, and afterwards we would look into that again.

Sorry.

Cheers,
Roman

@JoHeinem
Copy link

JoHeinem commented Jul 6, 2017

Hi @langfr,

We get the following error in the weblogic integration tests: https://pastebin.com/YtJERZzY We were also able to reproduce the error locally. The error looks similar to the one described here: https://community.oracle.com/thread/2372338

Do you want to investigate how we might get around this problem and commit a new PR? So far we do not know, how to get around this problem. I think it would make sense if you try to tackle this issue and report back your insights here. Next week, I'll discuss this problem with Daniel and we'll see how we can accommodate you with the jackson datatype JSR310 module integration.

Best,
Johannes

@JoHeinem
Copy link

Hi Frank,

I'll close this PR for now. If you still wish to add this feature, we can reopen the ticket at any time or you open a new PR.

Best,
Johannes

@JoHeinem JoHeinem closed this Jul 26, 2017
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

3 participants