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

Retry on ChecksumError #797

Merged
merged 25 commits into from Mar 10, 2018

Conversation

Projects
None yet
2 participants
@wisechengyi
Copy link
Collaborator

wisechengyi commented Feb 28, 2018

Fix for #780

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Feb 28, 2018

will add some tests

@alexarchambault would you kindly let me know if I'm on the right track?

@alexarchambault
Copy link
Member

alexarchambault left a comment

I added a few comments. I'm ok with the idea.

case _: FileError.WrongChecksum =>
val badFile = localFile(artifact.url, cache, artifact.authentication.map(_.user))
badFile.delete()
Console.err.println(s"Bad file deleted: ${badFile.getAbsolutePath}")

This comment has been minimized.

@alexarchambault

alexarchambault Feb 28, 2018

Member

Use logger instead? (a method can be added to it)

This comment has been minimized.

@wisechengyi

wisechengyi Feb 28, 2018

Collaborator

Added

)
case err: FileError.NotFound =>
EitherT(Task.now[Either[FileError, File]](Left(err)))
}

This comment has been minimized.

@alexarchambault

alexarchambault Feb 28, 2018

Member

You should add an extra case case err => to handle the other FileError cases.

This comment has been minimized.

@wisechengyi

wisechengyi Feb 28, 2018

Collaborator

Done

@@ -16,6 +16,11 @@ sealed abstract class FileError(

object FileError {

final case class RetryExhausted(reason: String) extends FileError(

This comment has been minimized.

@alexarchambault

alexarchambault Feb 28, 2018

Member

Maybe return the last FileError instead of this one? Or keep the last error as a field of that one?

This comment has been minimized.

@wisechengyi

wisechengyi Feb 28, 2018

Collaborator

Used the former and logs "Retry exhausted..." in console.

@wisechengyi wisechengyi changed the title Retry on DownloadError and ChecksumError Retry on ChecksumError Feb 28, 2018

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Feb 28, 2018

looks like I cannot catch DownloadError within the scope of def file, and it may be managed by coursier.Cache#downloading. Hence removing handling DownloadError for now.

I was testing DownloadError with

fetch junit:junit:4.12 --no-default -r http://<invalid_url>
@@ -5,7 +5,6 @@ import coursier.Cache

final case class CacheOptions(
@Help("Cache directory (defaults to environment variable COURSIER_CACHE or ~/.coursier/cache/v1)")
@Short("C")

This comment has been minimized.

@wisechengyi

wisechengyi Mar 1, 2018

Collaborator

Removing this because conflicting with --classifier / -C. I think the annotation processor should throw an error when there are conflicts.

This comment has been minimized.

@alexarchambault

alexarchambault Mar 5, 2018

Member

I agree. A helper method could be added for that in case-app.

@@ -509,4 +509,9 @@ class TermDisplay(
}
}

override def log(content: String, currentTimeOpt: Option[Long]): Unit = {
updateRunnable.newEntry(content, CheckUpdateInfo(currentTimeOpt, None, isDone = true), content)
updateRunnable.removeEntry(content, success = true, fallbackMessage = content)(x => x)

This comment has been minimized.

@wisechengyi

wisechengyi Mar 1, 2018

Collaborator

Not entirely sure if this is the right way to log things. Please kindly advise.

This comment has been minimized.

@alexarchambault

alexarchambault Mar 5, 2018

Member

Let's keep an empty implem for now (with a more specific method), we'll see how we can report that (with progress bars and all) later…

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Mar 2, 2018

@alexarchambault this is ready for another look when you get the chance. thanks

@@ -37,7 +37,7 @@ object Cache {
}

// java.nio.charset.StandardCharsets.UTF_8 not available in Java 6
private val UTF_8 = Charset.forName("UTF-8")
val UTF_8: Charset = Charset.forName("UTF-8")

This comment has been minimized.

@alexarchambault

alexarchambault Mar 5, 2018

Member

Just FYI, with Java >= 7, java.nio.charset.StandardCharsets.UTF_8 is available for that. I think I'll push a commit using the latter everywhere soon (while dropping support for Java 6 & 7). You can use it right now if you'd like.

@@ -518,7 +518,7 @@ object Cache {
doTouchCheckFile(file)
Right(false)
}
case other =>
case other =>FileError

This comment has been minimized.

@alexarchambault

This comment has been minimized.

@wisechengyi

wisechengyi Mar 5, 2018

Collaborator

Reverted. This is weird...CI passed on that.

validateChecksum(artifact, shaType, cache, pool).map(_ => file0)
}.leftFlatMap {
case err: FileError.WrongChecksum =>
if (retry == 0) {

This comment has been minimized.

@alexarchambault

alexarchambault Mar 5, 2018

Member

<= 0 just in case?

This comment has been minimized.

@wisechengyi

wisechengyi Mar 5, 2018

Collaborator

Done

@@ -1134,6 +1166,7 @@ object Cache {
def downloadedArtifact(url: String, success: Boolean): Unit = {}
def checkingUpdates(url: String, currentTimeOpt: Option[Long]): Unit = {}
def checkingUpdatesResult(url: String, currentTimeOpt: Option[Long], remoteTimeOpt: Option[Long]): Unit = {}
def log(content: String, currentTimeOpt: Option[Long]): Unit = {}

This comment has been minimized.

@alexarchambault

alexarchambault Mar 5, 2018

Member

I think it'd be better to have a more specific method for the error we're reporting… like

def removedCorruptFile(url: String, file: File, reason: Option[FileError]): Unit
@@ -509,4 +509,9 @@ class TermDisplay(
}
}

override def log(content: String, currentTimeOpt: Option[Long]): Unit = {
updateRunnable.newEntry(content, CheckUpdateInfo(currentTimeOpt, None, isDone = true), content)
updateRunnable.removeEntry(content, success = true, fallbackMessage = content)(x => x)

This comment has been minimized.

@alexarchambault

alexarchambault Mar 5, 2018

Member

Let's keep an empty implem for now (with a more specific method), we'll see how we can report that (with progress bars and all) later…

@@ -5,7 +5,6 @@ import coursier.Cache

final case class CacheOptions(
@Help("Cache directory (defaults to environment variable COURSIER_CACHE or ~/.coursier/cache/v1)")
@Short("C")

This comment has been minimized.

@alexarchambault

alexarchambault Mar 5, 2018

Member

I agree. A helper method could be added for that in case-app.

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Mar 7, 2018

plan to merge this in 24 hrs

}
else {
val badFile = localFile(artifact.url, cache, artifact.authentication.map(_.user))
badFile.delete()

This comment has been minimized.

@alexarchambault

alexarchambault Mar 7, 2018

Member

This call should be wrapped in a S.schedule(pool) { … }, as it's doing blocking stuff, like

S.schedule(pool) {
  badFile.delete()
  logger.foreach(…)
}.flatMap { _ =>
  file(…)
}

This comment has been minimized.

@wisechengyi

wisechengyi Mar 10, 2018

Collaborator

done

@@ -1123,6 +1155,8 @@ object Cache {

def gettingLength(url: String): Unit = {}
def gettingLengthResult(url: String, length: Option[Long]): Unit = {}

def removedCorruptFile(url: String, file: File, reason: Option[String]): Unit

This comment has been minimized.

@alexarchambault

alexarchambault Mar 7, 2018

Member

Let's keep the full original FileError in reason instead of a String.

This comment has been minimized.

@wisechengyi

wisechengyi Mar 10, 2018

Collaborator

done

@wisechengyi wisechengyi merged commit a7605ff into coursier:master Mar 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wisechengyi wisechengyi deleted the wisechengyi:retry_bad_sha branch Mar 10, 2018

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