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

2.1.x-Backport #741

Closed
wants to merge 1 commit into from
Closed

2.1.x-Backport #741

wants to merge 1 commit into from

Conversation

mkarg
Copy link
Contributor

@mkarg mkarg commented Feb 10, 2019

This PR is a backport of select bug fixes between 2.1.5 and master:HEAD into 2.1.6-SNAPSHOT (EE4J_8).

As these are no API changes, and as all these changes are already contained in master I propose a fast-track review period of just one day as per our recently update committer rules.

@mkarg mkarg added the bug Something isn't working label Feb 10, 2019
@mkarg mkarg added this to the 2.1.6 milestone Feb 10, 2019
@mkarg mkarg self-assigned this Feb 10, 2019
@mkarg
Copy link
Contributor Author

mkarg commented Mar 6, 2019

@mszabo-wikia Can you please renew your Eclipse Contributor Agreement?

Copy link

@asoldano asoldano left a comment

Choose a reason for hiding this comment

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

@mkarg , I'm not sure we should really be merging this changes to EE4J_8 branch. Wouldn't that mean breaking backward compatibility? We would end up with EE8 and JakartaEE8 having different behavior when it comes to MediaType#isCompatible, wouldn't we?

@mkarg
Copy link
Contributor Author

mkarg commented Mar 19, 2019

@asoldano Yes the behaviour would be changed, but in the way of a bug fix, so I think it is correct to backport it.

@mkarg mkarg force-pushed the 2.1.x-backports branch 2 times, most recently from 79ac315 to dd4b09a Compare April 20, 2019 15:08
@mkarg mkarg force-pushed the 2.1.x-backports branch from dd4b09a to f2eed18 Compare July 6, 2019 13:27
@mkarg mkarg changed the title WIP: 2.1.x-Backport 2.1.x-Backport Jul 6, 2019
@mkarg
Copy link
Contributor Author

mkarg commented Jul 6, 2019

Now that we have reactivated the branch EE4J_8 we should take care to keep it up-to-date wrt to bug fixes and clarifications. Hence I propose to cherry-pick (at least) the these two commits attached. If you think we should add more, let's keep things simple: Instead of having one PR per commit to cherry-pick, you can simply send a commit number to add to this PR. So we only need one voting for all. :-)

Regarding the Travis fail: This is a bug of Travis, I already opened a ticket with their support.

@mkarg
Copy link
Contributor Author

mkarg commented Jul 7, 2019

Thanks to Travis CI Support! All checks now have passed now.

chkal
chkal previously approved these changes Jul 8, 2019
Copy link
Contributor

@chkal chkal left a comment

Choose a reason for hiding this comment

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

+1 for backporting both commits.

@spericas
Copy link
Contributor

spericas commented Jul 8, 2019

Although these are really good changes and clarifications, I'm on the fence about pushing them to Jakarta EE 8. As I understand it, existing EE 8 implementations will not be updated for Jakarta EE 8, and the clarification regarding MBR's and MBW's may result in existing implementations to be "incompatible". I'm less worried about the other changes, but I see @asoldano has some concerns.

@mkarg mkarg force-pushed the 2.1.x-backports branch 2 times, most recently from 9cc9255 to ecda4bc Compare July 8, 2019 20:51
@mkarg
Copy link
Contributor Author

mkarg commented Jul 8, 2019

@spericas We had the discussion already, and the majority was for backporting it into Jakarta EE 8. The justification was that the existing products are not incompatible but simply buggy. Hence they SHOULD get fixed, but won't lose their compliance state even if not.

@chkal
Copy link
Contributor

chkal commented Jul 9, 2019

@spericas You already agreed to backport those fixes. See #700 (comment) and #711 (comment). Any reason for changing your opinion about this?

@spericas
Copy link
Contributor

spericas commented Jul 9, 2019

@chkal Yes, it's in my comment above. I didn't considered at the time that only API jars will be touched for Jakarta EE 8. Also note that I didn't vote -1, but simply shared the concern from @asoldano.

@spericas
Copy link
Contributor

spericas commented Jul 9, 2019

@spericas We had the discussion already, and the majority was for backporting it into Jakarta EE 8. The justification was that the existing products are not incompatible but simply buggy. Hence they SHOULD get fixed, but won't lose their compliance state even if not.

If you get the votes, you should push it. I'm +0.

@mkarg
Copy link
Contributor Author

mkarg commented Jul 9, 2019

@spericas @chkal Let's ask the one why is affected mostly...: @asoldano What is your vote on this?

@asoldano
Copy link

@mkarg @spericas @chkal please give me a bit of time to check a couple of things here before answering.

@jimma
Copy link
Contributor

jimma commented Jul 11, 2019

If this change will break backward compatibility somehow, I think we'd better to only fix this in 2.2 and
don't back port to 2.1.x branch. This will be helpful for user migrate to JakartaEE8 with less pain. If user's application, provider or filer uses jaxrs 2.1 version isCompatible() works, it's better that this still works in JakartaEE8 without change their code and rebuild all things.

@spericas
Copy link
Contributor

@asoldano If you can provide some feedback today, it would help with the release process. Thanks.

@asoldano
Copy link

I've been thinking about this for some time and frankly I'm still concerned about backward compatibility here. My understanding is that JakartaEE 8 is expected to be equivalent to JavaEE 8. Vendors are most likely going to "move" to JakartaEE in minor releases, as it comes with no new features. Users will be expecting the same behaviour out of their applications after having moved to JakartaEE 8. Because of that, the change to MediaType is risky, as an application might be directly relying on it. This looks like an edge case and not really a critical fix, which is what would justify the merge on EE4J_8 branch.
As for the other change, as long as the TCK does not enforce the clarified behaviour and hence implementations that have been assuming a different behaviour so far would still be considered as compliant, I would be fine with merging. Even if that's not a great situation (spec implementors would have to be fixed while having a configuration option for retaining the previous backward compatible behavior), at least user application wouldn't be affected by default.
As a consequence, despite the changes here being good, I'm sorry but think I need to cast a -1 to this PR. These changes fit 2.2+ IMO.

Copy link

@asoldano asoldano left a comment

Choose a reason for hiding this comment

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

-1, as per my previous comment

@spericas
Copy link
Contributor

Thanks for the feedback @asoldano. In that case, I think we should close this PR. Do you agree @mkarg?

@mkarg
Copy link
Contributor Author

mkarg commented Jul 11, 2019

@spericas I agree to drop all commits that are -1, but do I understand that @asoldano is OK with eacb77e. Is that correct? So I would reduce the PR to that one.

@mkarg mkarg force-pushed the 2.1.x-backports branch from ecda4bc to 7cd3b73 Compare July 11, 2019 17:07
@mkarg
Copy link
Contributor Author

mkarg commented Jul 11, 2019

@asoldano @chkal Dropped the disputed commited from the PR. LGTY?

@asoldano
Copy link

@mkarg I still see the MediaType changes included here.

@mkarg mkarg force-pushed the 2.1.x-backports branch from 7cd3b73 to ca61670 Compare July 11, 2019 20:54
@mkarg
Copy link
Contributor Author

mkarg commented Jul 11, 2019

@mkarg I still see the MediaType changes included here.

Sorry, my fault. Fixed it. Thanks! :-)

@spericas
Copy link
Contributor

@mkarg Once we get this one in, I'd like to initiate the release process and work with the TCK team.

@mkarg
Copy link
Contributor Author

mkarg commented Jul 12, 2019

@spericas Agreed. @asoldano +1?

@asoldano
Copy link

@mkarg @spericas for the MBR/MBW related change, my comment (#741 (comment)) still applies. So assuming the TCK won't be enforcing this, I'm +0 on this and we can proceed, considering approvals from others ;-)

@mkarg
Copy link
Contributor Author

mkarg commented Jul 12, 2019

Alessio's vote means that the backport will not happen, as we do not have enough +1's. @spericas Do not wait for this PR, it will not make it in time.

@mkarg
Copy link
Contributor Author

mkarg commented Jul 12, 2019

@eclipse-ee4j/eclipse-jaxrs I'm a bit disappointed to see that the majority of committers do not even vote with 0, but completely abstain from casting votes at all. :-(

@mkarg
Copy link
Contributor Author

mkarg commented Jul 12, 2019

@asoldano Can you please remove your -1 as we fixed your requested changes?

Copy link

@asoldano asoldano left a comment

Choose a reason for hiding this comment

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

+0

@asoldano asoldano dismissed their stale review July 12, 2019 20:43

Fixed

@spericas
Copy link
Contributor

Given that it has not received the necessary votes, I will initiate the release process for Jakarta EE 8 without this PR. We can later decide to close it or revisit this discussion (if another release is necessary).

@mkarg mkarg modified the milestones: 2.1.6, 2.2 Dec 18, 2019
@mkarg mkarg closed this Dec 28, 2019
@mkarg mkarg deleted the 2.1.x-backports branch December 30, 2019 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants