Use tar and gzip to compress+archive shipped jars #2
Conversation
| val usedFileNames = mutable.HashSet.empty[String] | ||
| for (path <- paths) { | ||
| val file = new File(path) | ||
| if (!file.isFile) { |
There was a problem hiding this comment.
Does leveraging the tar API make it easy to provide a directory, and have it send all the files underneath that directory? I can imagine that might be a nice feature, although it increases the likelihood of accidentally grabbing a lot of data that wasn't intended.
Related question: is useful to allow a configured hard limit on tarball size?
There was a problem hiding this comment.
We could do this but we need to add the logic to recursively add the files in directories. I think what we have here will cover enough use cases; I recall that the jars API in general doesn't allow for adding directories.
There was a problem hiding this comment.
I'm not sure about the usefulness of a configured limit mostly because the size that matters is the size after compression which could be difficult for application submitters to predict.
|
|
||
| def unpackAndWriteCompressedFiles( | ||
| compressedData: TarGzippedData, | ||
| rootOutputDir: File): Seq[String] = { |
There was a problem hiding this comment.
Is root output directory a configuration parameter?
There was a problem hiding this comment.
They're dynamically created in temp space.
|
We can wait to see if anybody else has questions, but LGTM |
mccheah
force-pushed
the
k8s-support-alternate-incremental--compress-jars
branch
from
aeb4351 to
ee07588
Compare
ash211
left a comment
ash211
left a comment
There was a problem hiding this comment.
I like it! Some requests for better comments and a warning log line but nothing significant
| var deduplicationCounter = 1 | ||
| while (usedFileNames.contains(resolvedFileName)) { | ||
| resolvedFileName = s"$nameWithoutExtension-$deduplicationCounter.$extension" | ||
| deduplicationCounter += 1 |
There was a problem hiding this comment.
I'd be a fan of logging a warning here -- having multiple jars on a classpath with the same name is bad practice anyway, especially if their contents are different...
|
|
||
| private[spark] object CompressionUtils { | ||
| private val BLOCK_SIZE = 10240 | ||
| private val RECORD_SIZE = 512 |
There was a problem hiding this comment.
add comment that these are the defaults from TarArchiveOutputStream
| val paths = mutable.Buffer.empty[String] | ||
| val compressedBytes = Base64.decodeBase64(compressedData.dataBase64) | ||
| if (!rootOutputDir.exists) { | ||
| rootOutputDir.mkdir |
There was a problem hiding this comment.
does this need to be a mkdir -p -- create intermediate directories?
| private val RECORD_SIZE = 512 | ||
| private val ENCODING = CharsetNames.UTF_8 | ||
|
|
||
| def createTarGzip(paths: Iterable[String]): TarGzippedData = { |
There was a problem hiding this comment.
add comment to this method that any folder hierarchy on the input paths is flattened and duplicate filenames have a _N suffix added before the extension
There was a problem hiding this comment.
Folder hierarchies actually aren't allowed, an exception is thrown instead. Still not sure if that's the right call however.
There was a problem hiding this comment.
Ah never mind, we want to note that only the file names are extracted from the full folder paths.
| ) | ||
| } | ||
|
|
||
| def unpackAndWriteCompressedFiles( |
There was a problem hiding this comment.
scaladoc that the return value is a seq of absolute file paths in their written location
| */ | ||
| package org.apache.spark.deploy.kubernetes.integrationtest; | ||
|
|
||
| public class PiHelper { |
There was a problem hiding this comment.
is this class primarily pulled out from SparkPiWithInfiniteWait.scala so there are multiple jars? Would be worth saying that in comments
|
Merge conflicts now after the folder rename |
|
@mccheah, sanity-check: the merge from |
|
Generally, it is cleaner to re-base for keeping updated, than merging. |
…cremental' into compress-jars
|
Still LGTM |
* Use tar and gzip to archive shipped jars. * Address comments * Move files to resolve merge
* Use tar and gzip to archive shipped jars. * Address comments * Move files to resolve merge
* Use tar and gzip to archive shipped jars. * Address comments * Move files to resolve merge
Augments #1 with compressing the user's jar uploads before they are sent over the wire. Note that while gzipping is the important part to compress things, there could be multiple possible choices over tar as the format for indicating "boundaries" between items in the stream. Encoding the file names is tricky to do by hand however so being able to get this for free using tar is helpful.