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 option to dump export tables for slave nodes. #18

Merged
merged 1 commit into from Aug 23, 2015

Conversation

Projects
None yet
9 participants
@christ66
Member

christ66 commented Jan 9, 2015

Could be useful for detecting certain types of memory leaks.

@christ66 christ66 changed the title from Add option to dump export tables for slave nodes. Could be useful for de... to Add option to dump export tables for slave nodes. Jan 9, 2015

@jenkinsadmin

This comment has been minimized.

Member

jenkinsadmin commented Jan 9, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@christ66

This comment has been minimized.

Member

christ66 commented Jan 27, 2015

@tfennelly

This comment has been minimized.

Member

tfennelly commented Jan 27, 2015

looks good 👍

public class SupportTestUtils {
/**
* Create a container from an output stream. Test units can use this to generate a

This comment has been minimized.

@jglick

jglick Jan 27, 2015

Member

Truncated comment.

}
}
};
}

This comment has been minimized.

@jglick

jglick Jan 27, 2015

Member

Would be more convenient to take a Component and return a String (via ByteArrayOutputStream).

@jglick

This comment has been minimized.

Member

jglick commented Jan 27, 2015

Looks like it could be useful. The main risk is that the export table is huge and swamps the bundle. Is it in practice limited to some reasonable size by the way Remoting is used?

@christ66

This comment has been minimized.

Member

christ66 commented Jan 27, 2015

Truncate would be nice, but might not capture everything unfortunately.

@christ66

This comment has been minimized.

Member

christ66 commented Jan 27, 2015

@jglick Would it make sense for us to remove duplicates instead of truncating?

@jglick

This comment has been minimized.

Member

jglick commented Jan 28, 2015

Would it make sense for us to remove duplicates instead of truncating?

Sure. Maybe nothing needs to be done—maybe the output as it stands is naturally limited to only a couple dozen lines or so. I am just asking what environments you have tested this in and whether you can be confident that the output will be reasonable, or whether some kind of truncation/deduplication might be necessary.

* Dump export table of nodes to detect potential
* memory leaks.
*
* User: schristou88

This comment has been minimized.

@stephenc

stephenc Jan 28, 2015

Member

replace the template comments with an @SInCE tag

@stephenc

This comment has been minimized.

Member

stephenc commented Jan 28, 2015

It might help to put a time bound for each node and truncate if the time bound is exceeded... though as this is a local only operation from my reading of the code that may be an unnecessary concern

@stephenc

This comment has been minimized.

Member

stephenc commented Mar 2, 2015

@christ66 where are you with closing this one out?

@stephenc

This comment has been minimized.

Member

stephenc commented Mar 16, 2015

@christ66 ping!

@jglick jglick changed the title from Add option to dump export tables for slave nodes. to [WiP] Add option to dump export tables for slave nodes. May 12, 2015

@jglick

This comment has been minimized.

Member

jglick commented May 12, 2015

Seems to be stalled.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jun 20, 2015

@christ66 Is this work in progress or can it be reviewed?

@christ66

This comment has been minimized.

Member

christ66 commented Jun 25, 2015

@jglick @stephenc I added a TruncateComponent class which should handle the exportTable dump being very large. I wrote a unit test to confirm that the export table can contain duplicate content, and could grow to be very large. The test also confirms that my TruncateComponent will properly truncate the export table dump if it grows large. I believe if the export table size is over 2MB in size that we would most likely have enough information to determine what could potentially be causing a memory leak.

@daniel-beck I am removing the WIP and it can be reviewed.

@christ66 christ66 changed the title from [WiP] Add option to dump export tables for slave nodes. to Add option to dump export tables for slave nodes. Jun 25, 2015

@reviewbybees

This comment has been minimized.

reviewbybees commented Jul 1, 2015

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 find out
more about our process please see this note

}
}
protected abstract void printTo(PrintWriter out) throws IOException;

This comment has been minimized.

@jtnord

jtnord Jul 2, 2015

Member

as this is intended to be implemented in sub-classes a little javadoc would go a long way...

@stephenc

This comment has been minimized.

Member

stephenc commented Jul 6, 2015

🐛

The problem that I have with the current truncated content and output stream implementation is that they do not stop the iteration of all the objects and potentially costly outputting of allocation and release stack traces... all they do is limit the size of content pushed to the bundle.

What I would do with truncation is actually throw an exception (subclassing IOException) when the limit is reached so that the write method bombs out (you can then catch the specific TruncationException and `//ignore`` it)

@christ66

This comment has been minimized.

Member

christ66 commented Jul 10, 2015

@stephenc I implemented the TruncationException. Instead of ignoring I added a logging event that it happened.

@stephenc

This comment has been minimized.

Member

stephenc commented Jul 10, 2015

🐝

@Override
public void addContents(@NonNull Container result) {
for (final Node node : Jenkins.getInstance().getNodes()) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 13, 2015

Member

Minor: Jenkins.getInstance() may return null

import java.io.IOException;
/**
* User: schristou88

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 13, 2015

Member

If you create Javadocs, please add information details there. Date and Time are useless, use @since to refer the plugin version, where the fix will be released

public void write(int b) throws IOException {
if (max > currentSize) {
out.write(b);
currentSize++;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 13, 2015

Member

You start counting bytes from 0, so the check is not correct. If max is 8000, 8001 bytes can be written

/**
* Content added to the support bundle that should be truncated.
*
* @author schristou88

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 13, 2015

Member

Needs @since, because it's a useful API class

This comment has been minimized.

@christ66

christ66 Jul 14, 2015

Member

Fixed.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jul 13, 2015

🐛 (ranges check)

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jul 14, 2015

🐝
Could you squash the commits into a single one?

@christ66

This comment has been minimized.

Member

christ66 commented Aug 15, 2015

@oleg-nenashev squashed!

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Aug 16, 2015

🐝 again

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Aug 22, 2015

🐝

@christ66

This comment has been minimized.

Member

christ66 commented Aug 23, 2015

christ66 added a commit that referenced this pull request Aug 23, 2015

Merge pull request #18 from christ66/exportTable
Add option to dump export tables for slave nodes.

@christ66 christ66 merged commit 5e5cfff into jenkinsci:master Aug 23, 2015

1 check passed

Jenkins This pull request looks good
Details

@christ66 christ66 deleted the christ66:exportTable branch Aug 23, 2015

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