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

[JENKINS-49588] add readResolve method for 'usages' in Fingerprint.java #3305

Merged
merged 2 commits into from Feb 1, 2019

Conversation

@mikecirioli
Copy link
Contributor

mikecirioli commented Feb 20, 2018

add readResolve method to ensure that <usages ../> is always initialized properly when deserializing a fingerprint

See JENKINS-49588.

  • It is not clear how this situation (a Fingerprint file without any usages being serialized to disk) is occuring, but this fix will prevent it from blowing up in the event that it does happen.

Proposed changelog entries

  • Entry 1: Internal: Added readResolve method to Fingerprint.java to ensure the usages field is always initialized

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees @batmat @oleg-nenashev

always initialized properly when deserializing a fingerprint
@mikecirioli
Copy link
Contributor Author

mikecirioli commented Feb 20, 2018

@oleg-nenashev - i have a couple of questions about this, i will follow up with you via email or chat.

@oleg-nenashev oleg-nenashev requested a review from batmat Feb 21, 2018
@oleg-nenashev
Copy link
Member

oleg-nenashev commented Feb 21, 2018

@reviewbybees it's on your plate

@batmat
Copy link
Member

batmat commented Feb 21, 2018

@mikecirioli I think a test (probably using @LocalData) is required here for completeness.

Also, added the WIP label. Please remember to remove (or ask for) it when you consider it ready for review.

that a fingerprint file without a <usages ...> can still be deserialized
@mikecirioli mikecirioli changed the title [WIP] [JENKINS-49588] add readResolve method for 'usages' in Fingerprint.java [JENKINS-49588] add readResolve method for 'usages' in Fingerprint.java Feb 22, 2018
@mikecirioli
Copy link
Contributor Author

mikecirioli commented Feb 22, 2018

please remove the work-in-progress label, this PR is ready for reiew

@daniel-beck
Copy link
Member

daniel-beck commented Feb 22, 2018

It is not clear how this situation (a Fingerprint file without any usages being serialized to disk) is occuring

I'd really like to see more investigation related to this. Either we ship 2.107.1 without this fix, or with it, and neither seems particularly appealing.

@mikecirioli
Copy link
Contributor Author

mikecirioli commented Feb 22, 2018

@daniel-beck I agree, although I am not sure how to go about finding a way to reproduce it. I searched jira for any possibly related issues but found some issues with fingerprints being truncated due to "&" characters in text strings. If you have ideas for ways to recreate the root cause I am happy to investigate.

@batmat
batmat approved these changes Mar 4, 2018
Copy link
Member

batmat left a comment

LGTM, cannot see how this could hurt, though agreed it would be better to understand where this comes from, if possible.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Mar 17, 2018

@batmat @mikecirioli do you plan to merge it?

@batmat
Copy link
Member

batmat commented Mar 18, 2018

How about we add a WARNING in the if?

That could be useful to know if this a common issue, and in the future (if not everybody upgraded to this in the meantime, granted) this would be sent back through telemetry for Essentials.

@batmat
Copy link
Member

batmat commented Mar 28, 2018

I'll merge later today if nobody objects.

@daniel-beck
Copy link
Member

daniel-beck commented Mar 28, 2018

Still lacking any understanding why this happens. Have we really made no progress there in the last five weeks?

@mikecirioli
Copy link
Contributor Author

mikecirioli commented Apr 5, 2018

another person reported seeing this in JENKINS-49588, are we still planning to merge?

@daniel-beck
Copy link
Member

daniel-beck commented Apr 7, 2018

Based on recent feedback in JENKINS-49588 it looks to me like we're lacking appropriate diagnostics for malformed fingerprint files. Is there a reasonable way to do that?

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jun 17, 2018

@mikecirioli could you please respond to the question above?

@mikecirioli
Copy link
Contributor Author

mikecirioli commented Jun 19, 2018

@daniel-beck @oleg-nenashev - this has dropped off my radar as of late. As far as I am aware we do not have a better understanding of why this is happening.

@mikecirioli
Copy link
Contributor Author

mikecirioli commented Jul 30, 2018

Can we re-consider merging this PR? Even if we don't understand what is causing this issue, i am still seeing comments from people who are experiencing it. This PR should at least soothe some of that pain. I can add the warning that @batmat suggested if that will help us get some additional datapoints.

@batmat batmat removed the ready-for-merge label Jan 9, 2019
Copy link
Member

oleg-nenashev left a comment

I consider this pull request a harmless example of the protective code. It is not clear whether it addresses the reported issue, but it is definitely harmless as is. Since there is no movement expected here, I propose to merge it instead of closing

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jan 31, 2019

@batmat @daniel-beck do you agree?

@oleg-nenashev oleg-nenashev self-assigned this Jan 31, 2019
@batmat
Copy link
Member

batmat commented Jan 31, 2019

Yes, agree. At least it will help us triage the open JIRA: if the issue keeps coming after this merged, then this will already be an additional data point.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jan 31, 2019

OK. I will merge it into the next weekly if there is no negative feedback

@oleg-nenashev oleg-nenashev merged commit e091ea2 into jenkinsci:master Feb 1, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@mikecirioli mikecirioli deleted the mikecirioli:JENKINS-49588 branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.