Skip to content
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

Fixed type of exception thrown by validateTaps #1033

Merged
merged 3 commits into from
Sep 3, 2014
Merged

Fixed type of exception thrown by validateTaps #1033

merged 3 commits into from
Sep 3, 2014

Conversation

fgcallari
Copy link
Contributor

The API specifies that validateTaps should throw InvalidSourceException, rather than IllegalArgumentException

The API specifies that validateTaps should throw InvalidSourceException, rather than IllegalArgumentException
The API specifies that validateTaps should throw InvalidSourceException, rather than IllegalArgumentException
"Version %s does not exist. Currently available versions are: %s"
.format(version, store.getAllVersions))
}
}

case _ => throw new IllegalArgumentException(
case _ => throw new InvalidSourceException(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one isn't recoverable is it? do we want this one to be the old exception maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not have a choice, given the API specification (and there is code that depends on that
specification being observed). See https://github.com/twitter/scalding/blob/develop/scalding-core/src/main/scala/com/twitter/scalding/Source.scala#L154

@johnynek
Copy link
Collaborator

johnynek commented Sep 3, 2014

Agree that sources are supposed to throw InvalidSource in validateTaps.

Good catch.

johnynek added a commit that referenced this pull request Sep 3, 2014
Fixed type of exception thrown by validateTaps
@johnynek johnynek merged commit 79ac4ef into twitter:develop Sep 3, 2014
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.

3 participants