Skip to content

MNG-6276 reproducible builds : ensure ZipEntry order in ParallelScatterZipCreator#77

Closed
Arnaud-Nauwynck wants to merge 2 commits intoapache:masterfrom
Arnaud-Nauwynck:master
Closed

MNG-6276 reproducible builds : ensure ZipEntry order in ParallelScatterZipCreator#77
Arnaud-Nauwynck wants to merge 2 commits intoapache:masterfrom
Arnaud-Nauwynck:master

Conversation

@Arnaud-Nauwynck
Copy link
Copy Markdown

No description provided.

@coveralls
Copy link
Copy Markdown

coveralls commented May 5, 2019

Coverage Status

Coverage decreased (-0.05%) to 86.48% when pulling f827d6d on Arnaud-Nauwynck:master into e127b13 on apache:master.

return new OutputStream() {
@Override
public void write(int b) throws IOException {
byte[] data = new byte[] { (byte) b };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Allocating one-byte arrays? Won't this hurt performance?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi,
This method is necessary to implements OutputStream methods, and delegate to
ScatterGatherBackingStore.writeOut(byte[] data, int offset, int length) throws IOException;
Anyway, this method from this private adapter class is not used.

Maybe, It would be better that ScatterGatherBackingStore expose directly an OutputStream (but it would change signatures), or expose a unitary "write(byte data)"

Regards,

@hboutemy
Copy link
Copy Markdown
Member

hboutemy commented May 6, 2019

  1. I suppose we'll need to create a Commons Jira issue
  2. based on the same idea (we worked together with Arnaud at a Hackergarten event last week :) ), I just tried another implementation that changes less content hboutemy@8669213 : I still need to improve the iterator state management, to avoid the current fields and ensure close if any failure happens, but the spirit is there

@bodewig
Copy link
Copy Markdown
Member

bodewig commented Aug 7, 2019

Many thanks @Arnaud-Nauwynck I've decided to use @hboutemy PR #78 instead of your version.

@bodewig bodewig closed this Aug 7, 2019
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.

5 participants