Opened 4 years ago

Closed 3 years ago

#16424 closed enhancement (implemented)

Add code to eventually support .xz compressed tarballs

Reported by: karsten Owned by: karsten
Priority: Medium Milestone:
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: iwakeh Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We switched from providing .tar.bz2 files on CollecTor to .tar.xz around July or August 2014, but we never updated metrics-lib to parse those.

We should try out how slow parsing .xz compressed tarballs is compared to uncompressed tarballs. Maybe the better plan is to put out a big warning and suggest that users decompress themselves using the unxz tool.

When we add support for this, let's try not to add any new dependencies to metrics-lib.

Child Tickets

Attachments (11)

Benchmark16424.java (6.3 KB) - added by leeroy 4 years ago.
Benchmark straming reads from tar, tar.bz2, tar.xz.
Benchmark16424.2.java (6.3 KB) - added by leeroy 4 years ago.
Benchmark streaming reads from tar, tar.bz2, tar.xz.
0001-Support-parsing-of-xz-compressed-tarballs.2.patch (9.5 KB) - added by leeroy 4 years ago.
0001-Support-parsing-of-xz-compressed-tarballs.patch (9.2 KB) - added by leeroy 4 years ago.
jarconflicts.tar (50.0 KB) - added by karsten 4 years ago.
0001-Eventually-support-xz-tarballs.2.patch (7.2 KB) - added by leeroy 4 years ago.
Now based on Task 16424.
0001-Eventually-support-xz-tarballs.3.patch (8.0 KB) - added by leeroy 4 years ago.
Now based on metrics master
0001-Eventually-support-xz-tarballs.patch (8.3 KB) - added by leeroy 4 years ago.
Now based on Task 16424.
0001-Eventually-support-xz-tarballs.4.patch (9.1 KB) - added by leeroy 4 years ago.
Now based on metrics master
0001-Eventually-support-xz-tarballs.-task-16424.patch (8.4 KB) - added by leeroy 4 years ago.
from task-16424
0001-Eventually-support-xz-tarballs.-master.patch (9.1 KB) - added by leeroy 4 years ago.
from master

Download all attachments as: .zip

Change History (52)

comment:1 Changed 4 years ago by leeroy

It looks like streaming reads from tar.xz are faster than tar.bz2. A good sign. I'll make a benchmark for you.

Here's a benchmark. It might be a bit verbose but it shows that compared to tar.bz2 (previously), tar.xz is an improvement both in file size and streaming read performance. Note the benchmark provides some totals but doesn't make an attempt to hide read latency. If setup and read are separate from a dedicated thread for parsing it's conceivable to hide the read behind the parse.

I'll look at instrumenting metrics-lib next (on a private copy). I see a potential improvement which may be useful while adding the .xz support. Namely it looks like the read from the archive could be improved.

I tried to fix a typo in the file description, which is why you see two attachments. They're the same file.

Last edited 4 years ago by leeroy (previous) (diff)

Changed 4 years ago by leeroy

Attachment: Benchmark16424.java added

Benchmark straming reads from tar, tar.bz2, tar.xz.

Changed 4 years ago by leeroy

Attachment: Benchmark16424.2.java added

Benchmark streaming reads from tar, tar.bz2, tar.xz.

comment:2 Changed 4 years ago by karsten

Thanks for working on the benchmark code. Would you be able to attach some results?

I'm also curious about those other improvements you're mentioning.

Thanks!

comment:3 Changed 4 years ago by leeroy

Which other improvements? The parsing improvement or read from archive? If the parsing improvement, it's a surprise. As far as the read from archives is concerned, I'll paraphrase in pseudo-pseudocode what metrics-lib does versus what I do in the benchmark.

Metrics-lib

  1. (8k reads) get bufferedinputstream(get tararchiveinputstream(get fileinputstream from archive))
  2. (set position) get a tar entry corresponding to a contained descriptor file
  3. construct an unbounded bytearrayoutputstream of unknown size requiring JVM management
  4. read 1k from bufferedinputstream into a separate 1k buffer
  5. copy this 1k buffer into bytearrayoutputstream
  6. repeat (4) and (5) until all bytes of the tar entry are read
  7. now bytearrayoutputstream is a copy of the tar entry, convert bytearrayoutputstream to bytearray (make another copy)
  8. parse the second copy

Benchmark

  1. (8k reads) get archiveinputstream(get bufferedinputstream(get fileinputstream from archive))
  2. (set position) get an archive entry corresponding to a contained descriptor file
  3. construct a bounded byte array from the known size of the archive entry, a primitive which is only GC'd by JVM
  4. read the archive entry into the byte array
  5. parse the byte array

Maybe it doesn't matter much. Which is why I'll next be doing some testing of metrics-lib performance. I'll post some results once had a chance to do the tests on metrics-lib. Mostly the read is only noticeable during large archive entries. If you want to try it out put the partial 2015-07.tar.xz archives in the same folder as the benchmark, and make sure you have metrics-lib plus it's dependencies (libcommons-codec, libcommons-compress), then compile/run making sure to set your classpath.

javac -cp /usr/share/java/*:.:descriptor.jar Benchmark16424.java

java -cp /usr/share/java/*:.:descriptor.jar Benchmark16424

Last edited 4 years ago by leeroy (previous) (diff)

comment:4 Changed 4 years ago by leeroy

The patch reimplements the compressed tarball support and makes some attempts at improving buffering. It also requires a small change to the build file for metrics-lib.

Fixed the exception being set when a parse error happens.

Last edited 4 years ago by leeroy (previous) (diff)

comment:5 Changed 4 years ago by leeroy

Status: newneeds_review

comment:6 Changed 4 years ago by karsten

Status: needs_reviewneeds_revision

Hi leeroy,

I reviewed your 0001-Support-parsing-of-xz-compressed-tarballs.patch, pushed it to my public Git repository, and added some commits. Please take a look at these changes and let me know if you like them or not:

https://gitweb.torproject.org/user/karsten/metrics-lib.git/log/?h=task-16424

Here's some more feedback:

  • Sorry for being picky about whitespace and code style. I'm not at all saying that the styles used in metrics-lib are perfect. We can discuss code style changes if you feel strongly about something. I just think that being consistent is important to make the code more readable.
  • Can you create a metrics-lib (and Onionoo) repository somewhere (GitHub, Bitbucket, etc.) and post future patches as branches there? That would make it much easier to split changes into several commits.
  • I'm yet unclear whether we should raise the Java version from 6 to 7 for this change. The effect is that all applications using metrics-lib would suddenly have to upgrade, too. Can we postpone this upgrade? (We'd have to undo the try-with-resources part.)
  • That change to the Ant build file where you include dependent jar files in the produced jar file seems unrelated to adding parsing support for .xz files. It might be a good idea to do this, but can we postpone (as in: undo) this change, too?
  • I didn't test this branch yet. It's quite possible that I broke it.

Thanks for working on this! Much appreciated.

comment:7 Changed 4 years ago by leeroy

Thank you for the feedback. Please do be picky about whitespace and style.

Yes I will do as you suggest about the repository. The only reason I haven't sooner is due to using trac as a sort of backup.

Java 7 is additive wrt the api. Any 1.6 based applications will compile fine. But yes I can remove it. metrics-lib is exactly the kind of application that benefits from 1.7...

Which file are you referring to? xz.jar (libxz)? it is a required include. It's just done by libcommons-compress normally. This include makes the dependency clear. xz support is an optional download of libcommons-compress. This include detects it's absence.

Is it the other two includes, commons-codec.jar, and commons-compress.jar? They are also required includes for metrics-lib functionality. The only reason it hasn't failed sooner is because you only use it in onionoo (and onionoo does the include for you). For metrics-lib to be general these includes are needed...

Last edited 4 years ago by leeroy (previous) (diff)

comment:8 in reply to:  7 Changed 4 years ago by karsten

Replying to leeroy:

Thank you for the feedback. Please do be picky about whitespace and style.

Thanks for understanding!

Yes I will do as you suggest about the repository. The only reason I haven't sooner is due to using trac as a sort of backup.

Oh. :)

Java 7 is additive wrt the api. Any 1.6 based applications will compile fine. But yes I can remove it. metrics-lib is exactly the kind of application that benefits from 1.7...

I could imagine switching to 1.7 soon, I'd just prefer merging this patch even sooner. New ticket?

Which file are you referring to? xz.jar (libxz)? it is a required include. It's just done by libcommons-compress normally. This include makes the dependency clear. xz support is an optional download of libcommons-compress. This include detects it's absence.

Yes, the include is fine. I mean the <zipgroupfileset/> element.

Is it the other two includes, commons-codec.jar, and commons-compress.jar? They are also required includes for metrics-lib functionality. The only reason it hasn't failed sooner is because you only use it in onionoo (and onionoo does the include for you). For metrics-lib to be general these includes are needed...

Well, it would work if Onionoo (and others) would include xz.jar themselves, even without this change to the build file, right? This is also something we could change soon but which shouldn't be related to adding .xz parsing support. For example, one thing I'm concerned about is that we're using different commons-codec.jar versions for building descriptor.jar and as a dependency in Onionoo. It could well be that this concern is not valid, but I'd want to think more about that and any other implications when I have more time. New ticket?

comment:9 Changed 4 years ago by leeroy

I can't really comment about the use of libcommons-codec instead of a particular version. Would it help that most operators of metrics-lib will also be running newer versions than the version I develop with (which is from oldstable)? In general most operators will experience this dilemma as different distributions come with different versions. Personally I think it's not a problem as long as the code works with an oldstable version of libcommons (it does).

<zipgroupfileset/> is the way to import into jarspace. It doesn't work any other way. I'm concerned that you would prefer to import this into onionoo instead of metrics-lib where the feature is implemented.

If onionoo's build file were updated as in #16371 then onionoo wouldn't be using a different version.

Last edited 4 years ago by leeroy (previous) (diff)

comment:10 Changed 4 years ago by leeroy

Oh, I forgot one important detail about the xz archive support. It requires it's own core for optimal performance. The reads tend to finish before parsing it all though. Using file parsing, instead of xz archives, tends to be single-cpu bound because along the way metrics-lib blocks waiting for parse to complete. Is this intended?

Last edited 4 years ago by leeroy (previous) (diff)

comment:11 Changed 4 years ago by karsten

Regarding importing dependent jars into descriptor.jar, I can see pros and cons. Right now, we're producing a minimal descriptor.jar file and requiring applications to also add certain versions of other libraries into their classpath. With your change we'd produce a larger descriptor.jar that comes with the jars it needs, at the risk of producing uncertain behavior if applications use older or newer versions of those libraries in their classpaths. However, we don't have to solve this in this ticket, which is why I'd rather want to move this discussion to its own ticket. Sound okay?

With respect to your observation of blocking until parsing is complete, please take a look at DescriptorReader.setMaxDescriptorFilesInQueue and try setting it to a larger value. It's actually supposed to be a feature not to load and parse too many descriptors if the application doesn't process them. It's quite possible that there's a smarter way to achieve this. That discussion would go into a new ticket, too.

Let me summarize:

  • We should merge your patch without the 1.7 upgrade and without <zipgroupfileset/>. Want to undo these changes?
  • We should open two or three new tickets:
    • Upgrade to Java 1.7,
    • Include dependent jars in descriptor.jar, and
    • Find smarter metric than number of descriptor files to block loading and parsing until descriptors have been processed.

comment:12 Changed 4 years ago by leeroy

Yes, but implementing xz support without importing the libcommons-compress (and consequently libxz) may compile but means the compiled code doesn't work. So basically reading xz tarballs will fail spectacularly. ie. there's no way to implement this feature without that change. (Technically the parts of metrics-lib that depend on libcommons-codec would also fail spectacularly if not for onionoo's <zipgroupfileset/> include). What I mean is you developed metrics-lib to be general and contained. You're now creating a dependency in which all applications that use metrics-lib must know it's dependencies. metrics-lib cannot be both general and expect applications to know how it implements it's features. It's a bad idea.

About the DescriptorReader.setMaxDescriptorFilesInQueue , I will create a separate ticket about this.

Last edited 4 years ago by leeroy (previous) (diff)

comment:13 Changed 4 years ago by karsten

So, I just tried out what happens if we ship the jars that we depend on in our descriptor.jar: even if the application using our descriptor.jar provides different versions of those dependent jars by itself, it will get what comes with our jar. For example, if we ship a terribly old libcommons-codec, an application using our jar won't be able to use a recent enough version of libcommons-codec. To state this differently, we'd be forced to update the libcommons-codec in descriptor.jar even if nothing relevant changes for us, just to ensure that applications using us can use the latest and greatest. That's a dependency I'd rather not want to create. I'm attaching a tarball with a small example. Please read the Java code and Ant file and run the app.

Changed 4 years ago by karsten

Attachment: jarconflicts.tar added

comment:14 Changed 4 years ago by leeroy

metrics-lib depends on the soft links currently. The jars that actually get compiled depend on what they point to. metrics-lib runs on tor project infrastructure so it could be stable or oldstable. How can metrics-lib ship a terribly old libcommons when it depends on soft links? It will always be whatever is installed for meeting the dependency. metrics-lib doesn't ship libcommons.

If you want to merge xz support this is how it is. I didn't design the build system. I didn't make metrics-lib general. I can't change either of these. This is just a great big warning sign: If you merge without the build file modification (which uses soft link) you will be merging non-functional code.

comment:15 Changed 4 years ago by karsten

Well, oldstable can be terribly old for some. Let's assume I give you the new descriptor.jar that I compile on a Tor Project VM, and you'd want to use it for your project on testing where you're using both descriptor.jar and a very recent version of libcommons-codec with features that are not found in the oldstable version. Just because you're using descriptor.jar means you wouldn't be able to use them. That sounds bad.

Regarding your warning, isn't metrics-lib already non-functional code following that logic? If you fail to provide libcommons-codec and libcommons-compress, you'll see interesting errors.

How do other libraries do this? Do they all ship jars with all the other library jars they depend on? This problem cannot be new.

Are you around on IRC tomorrow? Maybe it's easier to talk about this than discuss here on the ticket.

comment:16 Changed 4 years ago by leeroy

Well if I needed a newer version of libcommons-codec for something I wouldn't get a jarfile from you. I would install the version of libcommons-codec I need (possibly an addition to the oldstable/stable version). After making sure the soft link is up to date with my needed version I would then compile the metrics-lib jar file myself and use that descriptor.jar with my application.

Regarding your warning, isn't metrics-lib already non-functional code following that logic? If you fail to provide libcommons-codec and libcommons-compress, you'll see interesting errors.

The comparison isn't accurate. If you fail to provide libcommons you don't have it installed. metrics-lib explicitly states it's a requirement. If you don't include libxz it simply won't work. libcommons-compress makes it optional.

Look it doesn't matter if you have libxz installed for it to work Karsten. It depends on including libxz in the build file. No include in the build file no xz tarball support...

Last edited 4 years ago by leeroy (previous) (diff)

comment:17 Changed 4 years ago by karsten

I wonder if we're talking about two different things. Let me paste a partial diff of build.xml:

@@ -11,6 +11,7 @@
       <include name="commons-codec.jar"/>
       <include name="commons-compress.jar"/>
       <include name="junit4.jar"/>
+      <include name="xz.jar"/>
     </fileset>
   </path>

This change must go in, or the code won't compile, right? (I didn't test this branch yet.) I'm not arguing against this change.

@@ -41,7 +42,13 @@
   </target>
   <target name="jar" depends="compile">
     <jar destfile="${jarfile}"
-         basedir="${classes}"/>
+         basedir="${classes}">
+      <zipgroupfileset dir="/usr/share/java">
+        <include name="commons-codec.jar"/>
+        <include name="commons-compress.jar"/>
+        <include name="xz.jar"/>
+      </zipgroupfileset>
+    </jar>
   </target>
   <target name="test" depends="compile">
     <javac destdir="${classes}"

This is the change I'd rather not want to merge for the reasons stated above.

Does that resolve any confusion?

comment:18 Changed 4 years ago by leeroy

The first change is optional, it just makes the dependency on libcommons-compress with libxz explicit. The code will compile without it. The second change is required to use xz tarballs. Onionoo already does codec and compress, so at a minimum libxz must be added to the build file or no xz tarball support will work. Not even if you have it installed.

comment:19 in reply to:  18 Changed 4 years ago by karsten

Status: needs_revisionneeds_review

Replying to leeroy:

The first change is optional, it just makes the dependency on libcommons-compress with libxz explicit. The code will compile without it.

Ah, okay. I'd say we can leave it in.

The second change is required to use xz tarballs. Onionoo already does codec and compress, so at a minimum libxz must be added to the build file or no xz tarball support will work. Not even if you have it installed.

Well, the xz.tarball certainly needs to be added to Onionoo's build file if we want Onionoo to support parsing xz tarballs with metrics-lib's help, but it doesn't need to be included in the jar file produced by metrics-lib's build file. I just tried it out, and that works just fine.

Please review my updated branch task-16424 and tell me if you're okay with squashing all those commits into yours and merging.

By the way, things don't break in too interesting ways if the xz.jar is not provided. Here's the exception you'd get:

Bug: uncaught exception or error while reading descriptors:
java.lang.NoClassDefFoundError: org/tukaani/xz/XZInputStream
	at org.torproject.descriptor.impl.DescriptorReaderImpl$DescriptorReaderRunnable.getArchiveInputStreamForFile(DescriptorReaderImpl.java:351)
	at org.torproject.descriptor.impl.DescriptorReaderImpl$DescriptorReaderRunnable.readTarballs(DescriptorReaderImpl.java:287)
	at org.torproject.descriptor.impl.DescriptorReaderImpl$DescriptorReaderRunnable.run(DescriptorReaderImpl.java:158)
	at java.lang.Thread.run(Thread.java:744)
Caused by: java.lang.ClassNotFoundException: org.tukaani.xz.XZInputStream
	at java.net.URLClassLoader$1.run(URLClassLoader.java:366)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:355)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:354)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:425)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:358)
	... 4 more

That's not pretty, but at least it explains what went wrong.

Once we have resolved this we should upgrade the metrics-lib in Onionoo and include xz.jar in its build file.

comment:20 Changed 4 years ago by leeroy

So you would rather have a separate ticket for build file changes. Okay.

comment:21 Changed 4 years ago by leeroy

Summary: Support parsing of .xz compressed tarballsAccept .xz compressed tarballs during parsing

comment:22 Changed 4 years ago by leeroy

Status: needs_reviewneeds_revision

comment:23 Changed 4 years ago by leeroy

Summary: Accept .xz compressed tarballs during parsingAdd code to eventually support .xz compressed tarballs

comment:24 Changed 4 years ago by leeroy

Status: needs_revisionneeds_review

comment:25 Changed 4 years ago by karsten

I'd prefer to discuss build file changes in a separate ticket, yes.

What's that latest patch for that you attached? If you want to make more changes, can you base them on my task-16424 branch which already contains fixes to your previous patch?

Note that I just pushed another commit to that branch, because I accidentally closed the stream in the catch block instead of the finally block, which doesn't make as much sense.

Ready to squash and merge my task-16424 branch?

comment:26 Changed 4 years ago by leeroy

because I accidentally closed the stream in the catch block instead of the finally block

If that's the case then I have no idea what change you've made. So I should test whatever you've done.

Your branch is wrong wrt setting the file name during tarball processing. You're setting the filename to the entry name including relative path within the archive. Previously it was the filename of the contained archive entry without the relative path.

You also have a bug where a failure to open an archive goes undetected.

Last edited 4 years ago by leeroy (previous) (diff)

Changed 4 years ago by leeroy

Now based on Task 16424.

comment:27 Changed 4 years ago by karsten

Hang on. That patch is re-applying changes from your original commit that I changed in squash commits. That doesn't make sense. I also think that I fixed the part where you were changing which file name gets set to what was set before. Please clone my branch task-16424 again and look at the diff from before your commit until now: git diff 2237e8935c307e21b8f0652e65a89c63ca939991... That's what's going to get squashed and merged.

Can you be more explicit about the bug where a failure to open an archive goes undetected? If there's such a bug we should certainly fix it. Thanks.

comment:28 Changed 4 years ago by leeroy

Karsten, I just did clone this branch.

  • You're using the archive entryName() not the parsed file name. Previously it was the parsed entry name minus the relative path.
  • You changed the archive stream helper to return a concrete tar archive input stream, previously it only returned a tar archive input stream if it was possible, and would return null otherwise. (before you ask, yes, it is possible to return an archive stream after failing to open) This now means an archive will return a non-null reference even if opening the archive failed. Which means operating on it's contents will eventually fail when it shouldn't have been processed.
Last edited 4 years ago by leeroy (previous) (diff)

comment:29 in reply to:  28 Changed 4 years ago by karsten

Replying to leeroy:

Karsten, I just did clone this branch.

  • You're using the archive entryName() not the parsed file name. Previously it was the parsed entry name minus the relative path.

Note that there are two different file names. We're setting the descriptor file name to the full path obtained from entry.getName(), and we're using the parsed entry name minus the relative path when we pass a file name to the descriptor parser. Again, I think I changed back what you changed beforehand.

  • You changed the archive stream helper to return a concrete tar archive input stream, previously it only returned a tar archive input stream if it was possible, and would return null otherwise. This now means an archive will return a non-null reference even if opening the archive failed. Which means operating on it's contents will eventually fail when it shouldn't have been processed.

I did change that helper method, but I don't think it would ever return a non-null reference in case of a failure. If there was an IOException that would be thrown and caught by the caller. I don't see what else could go wrong there, but maybe I'm just not seeing it.

comment:30 Changed 4 years ago by leeroy

I'm looking at the difference between your branch and what I submitted. Entry refers to the archive entry. entry.getName() returns the full canonical path name. It's filename that contains the parsed name.

I think you need to scrap the entire branch and look at it again. The original was designed to return null safely so that it could be detected without causing the application to fail. The branch you made for this isn't safe for use.

These two problems which were introduced in creating the branch are evidence of this.

Last edited 4 years ago by leeroy (previous) (diff)

Changed 4 years ago by leeroy

Now based on metrics master

comment:31 Changed 4 years ago by leeroy

An archive file not being opened as an archive is not a good reason to abort reading. The malformed archive may occur last. In which case your modification to the archive stream helper just wasted all the time spent parsing previous archives. Which may potentially be a lot.

(Aside: do also note metrics-lib currently has no way of indicating skipped archives to onionoo)

The only time when an IO failure during read should abort the read process is during recent file read. These files are expected to be properly formed. However, if this event occurs, the parse history for archives should have already been updated, so as to prevent the slim chance of data loss. This does not happen. This problem has been brought up in #16426.

(Specifically, all parse history is updated last during an onionoo update, so if something goes wrong you need to redo the parsing)

Last edited 4 years ago by leeroy (previous) (diff)

Changed 4 years ago by leeroy

Now based on Task 16424.

Changed 4 years ago by leeroy

Now based on metrics master

comment:32 Changed 4 years ago by leeroy

The updated patch now provides information on skipped archives during read. This will benefit applications that have no other way of detecting malformed archives. Onionoo will now be able to indicate to the operator if an archive happens to be corrupt.

Changed 4 years ago by leeroy

from task-16424

Changed 4 years ago by leeroy

from master

comment:33 Changed 4 years ago by leeroy

This updated patch tries to delay updating parsedFilesAfter until after the archive can be validated on opening. This ensures that when archive parse history is later added, it will not contain malformed archives. It also ensures that upon detecting and replacing the malformed archive, the fixed archive will be parsed as expected.

comment:34 Changed 4 years ago by leeroy

With respect to your observation of blocking until parsing is complete, please take a look at DescriptorReader.setMaxDescriptorFilesInQueue and try setting it to a larger value. It's actually supposed to be a feature not to load and parse too many descriptors if the application doesn't process them.

That's a problem.

  1. The descriptor queue contains parsed descriptors. Files are added after parsing.
  2. Parsing runs in the same thread, which explains why it's single-core bound.
  3. Total read time will always be less than total parse time, unless the data to be parsed is specifically selected to have low parse time.
  4. Parsing is slow. Slow enough that a queue of waiting data to be parsed can be readily formed.

I'll revisit this topic more in the separate ticket yet to be created.

comment:35 Changed 4 years ago by leeroy

Parent ID: #16614

comment:36 Changed 4 years ago by karsten

Your last patches change things back that I fixed before, e.g., you again changed entrySize back to entry_sz which does not conform with the code style in metrics-lib or even Java in general. Please trim your patches to the smallest size conceivable to facilitate the review process. And please push your branch to a public Git repository somewhere. Otherwise I'm inclined to squash and merge as is and address your concerns later or in different tickets. In that case, if you're uneasy to have your name and email address as the author of that squashed Git commit, let me know and I'll change the author and credit you in the commit message instead. Thanks.

comment:37 Changed 4 years ago by leeroy

No. No repository. You made this task-branch before submitting your review and request for revision. You didn't even wait for the revision. I know this is the case because you use entry.getName(). You should have reviewed, asked for revision, waited for revision, then created the task-branch.

Do not credit me for your task-branch. That is your choice alone.

  1. According to documentation it is not entry.getName() that should be used.
  2. If you're looking for the smallest patch for review that would be the one from master. Clearly labelled above. Because master actually works and your task-branch is really nothing more than frivolous style wrapped around an early (pre-revision) patch with modifications that completely break the functionality. If you wish for style changes you should first confirm you actually know what the patch does. Otherwise style means nothing as your task-branch demonstrates.

If you still consider your task branch as viable, and still have complaints about my style. Fine, I'll fix the style, when you fix your branch. Then maybe I'll point out the handful of places in your code where you fail miserably at anything remotely close to Java. You wouldn't like that though, so being generally nice, I won't.

I also don't see how having a repository would help. I see several outstanding bugs which you know about and have created tasks for. Those same bugs have been sitting there unfixed for > 3 months. You mention unfixed bugs in unrelated tickets but don't make an effort to apply your own fixes, and my repository would help?

  • Branch jessie upgrades a deprecated function call which has been deprecated since commons-codec 1.7. Not a fix.
  • Branch jessie upgrades the build file to libraries which are included in debian 8. This excludes any other OS including oldstable. The actual functionality, however, works on every other supported version of libcommons so all you've done is exclude every other OS. Not a fix.
  • GSON fixes are unapplied.
  • NodeStatus fixes are unapplied. Does not address the rest of the bugs produced during update.

You're either oblivious to your own "fixes" when you discuss the bugs in unrelated tickets, or you've a habit of pretense. At least I'm consistent.

edited: for meaningless style

Last edited 4 years ago by leeroy (previous) (diff)

comment:38 Changed 4 years ago by leeroy

Parent ID: #16614

comment:39 Changed 3 years ago by karsten

Cc: iwakeh added
Severity: Normal

I just pushed a new branch task-16424-2 to my public repository that contains a minimal patch to support parsing .xz-compressed tarballs and that I'll merge to master in the next few days.

comment:40 Changed 3 years ago by iwakeh

All fine.

comment:41 Changed 3 years ago by karsten

Resolution: implemented
Status: needs_reviewclosed

Great, thanks for looking! Merged (after rebasing and fixing some minor conflicts in CHANGELOG.md). Resolving.

Note: See TracTickets for help on using tickets.