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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-47896] - Introduce SerializableOnlyOverRemoting in classes. #3144

Closed
wants to merge 6 commits into from
Closed

[JENKINS-47896] - Introduce SerializableOnlyOverRemoting in classes. #3144

wants to merge 6 commits into from

Conversation

oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented Nov 16, 2017 •
edited
Loading

Due to the implementation specifics, some classes in the core are serializable only over Remoting.
This change just marks these classes and utilizes the convenience method in the interface for serialization/deserialization operations. The interface is available since Remoting 3.14.

See JENKINS-47896.

No tests, because the behavior does not change.
The error propagation just becomes better in edge cases.

Proposed changelog entries

  • N/A

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

@oleg-nenashev
[JENKINS-47896] - Introduce SerializableOnlyOverRemoting in classes.
bfeb24f 
Due to the implementation specifics, some classes in the core are serializable only over Remoting.
This change just marks these classes and utilizes the convenience method in the interface for serialization/deserialization operations.
@ghost
Copy link

ghost commented Nov 16, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Nov 16, 2017
@oleg-nenashev oleg-nenashev requested review from jglick and a user November 16, 2017 16:35
jglick
jglick approved these changes Nov 17, 2017
View reviewed changes
core/src/main/java/hudson/FilePath.java Outdated Show resolved Hide resolved
ghost
ghost approved these changes Nov 17, 2017
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

🐞

@oleg-nenashev
Copy link
Member Author

Fixed the merge conflict after the change by @jglick in #3122

@daniel-beck
Copy link
Member

Still (or again) conflicting.

@daniel-beck daniel-beck added the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 5, 2017
@oleg-nenashev oleg-nenashev mentioned this pull request Dec 26, 2017
Merged
3 tasks
@oleg-nenashev oleg-nenashev removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jan 28, 2019
@oleg-nenashev
Copy link
Member Author

Fixed the merge conflict

@oleg-nenashev oleg-nenashev removed the needs-more-reviews Complex change, which would benefit from more eyes label Jan 28, 2019
@oleg-nenashev oleg-nenashev self-assigned this Jan 28, 2019
jglick
jglick requested changes Jan 28, 2019
View reviewed changes
core/src/main/java/hudson/FilePath.java Outdated Show resolved Hide resolved
@oleg-nenashev
Copy link
Member Author

So, tests fail due to nulls:

java.lang.IllegalStateException: Can't send a remote FilePath to a different remote channel (current=null, target=hudson.remoting.Channel@5c278e52:The other side of the channel)

jglick
jglick reviewed Feb 4, 2019
View reviewed changes
core/src/main/java/hudson/FilePath.java Show resolved Hide resolved
core/src/main/java/hudson/cli/CliManagerImpl.java
@@ -47,7 +49,7 @@
* @deprecated Specific to Remoting-based protocol.
*/
@Deprecated
public class CliManagerImpl implements CliEntryPoint, Serializable {
public class CliManagerImpl implements CliEntryPoint, SerializableOnlyOverRemoting {
Copy link
Member

Choose a reason for hiding this comment

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

Do not bother, will just conflict with #3838.

core/src/main/java/hudson/util/StreamTaskListener.java
@@ -58,7 +60,7 @@
*
* @author Kohsuke Kawaguchi
*/
public class StreamTaskListener extends AbstractTaskListener implements TaskListener, Closeable {
public class StreamTaskListener extends AbstractTaskListener implements TaskListener, SerializableOnlyOverRemoting, Closeable {
Copy link
Member

Choose a reason for hiding this comment

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

I would expect this to be marked on TaskListener, not StreamTaskListener.

@batmat batmat added the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 4, 2019
@batmat
Copy link
Member

batmat commented Apr 4, 2019

@oleg-nenashev this PR is conflicted, are you willing to finalize it? Or you want to close it for now?
Spassiba!

@jglick
Copy link
Member

jglick commented Apr 8, 2019

If @oleg-nenashev does not have time, I think I could take this over—I understand the intent.

@batmat
Copy link
Member

batmat commented Apr 10, 2019

@jglick I guess this would be great if you can help here indeed. Given we're trying to reduce the number of open PRs, any help to push PRs to closure (be it merged or closed) is very much appreciated.

@jglick jglick mentioned this pull request Apr 11, 2019
Merged
@daniel-beck
Copy link
Member

Superseded by #3980

@jglick
Copy link
Member

jglick commented Apr 11, 2019

Yes, though not necessary to close this one—merging the larger will automatically merge this one as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick requested changes

Assignees

@oleg-nenashev oleg-nenashev

Labels
unresolved-merge-conflict There is a merge conflict with the target branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oleg-nenashev @daniel-beck @batmat @jglick