You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2015/08/23 21:51:57 UTC
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/1045
[FLINK-2557] TypeExtractor properly returns MissingTypeInfo
This fix is not really obvious so let me explain:
getParameterTye() is called from two different places in the TypeExtractor; to validate the input type and to extract the output type.
Both cases consider the possibility that getParameterType() fails, but check for different exceptions.
The TypeExtractor only returns a MissingTypeInfo if it encounters an InvalidTypesException; IllegalArgumentExceptions are not catched. This is what @mjsax encountered.
Changing the exception type causes the TypeExtractor to properly return a MissingTypeInfo, which is later overridden by the returns(...) call.
In order for the input validation to still work properly aswell, it now catches InvalidTypesExceptions instead.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink 2557_types
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1045.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1045
----
commit 1c1dc459915c875ab0a4412aa3ef0a844f092171
Author: zentol <s....@web.de>
Date: 2015-08-23T19:41:44Z
[FLINK-2557] TypeExtractor properly returns MissingTypeInfo
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-139234472
@fhueske well I'm still waiting for an answer as to how to proceed. Either return a MissingTypeInfo or throw some checked Exception when the type could not be determined. This is just a choice of style; both solutions require a pass over (probably) all calls to type extractor methods.
It might also be a good idea to separate the original fix (in the first commit) from the exception stuff. The fix did not fail any tests and resolved the issue, while the rest is essentially a refactoring.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-141402922
Merging this...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-135373495
An exception is probably preferable since it would be consistent with the getInfoFor() method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-140098455
I'm ok with this approach.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1045
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-141110503
Looks good to merge, if build succeeds.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-141024224
Can you add a test case that checks if "returns" works now. See the JIRA example.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-135364762
@tillrohrmann This distinction between checked and unchecked exceptions is a good point. It is not very consistent in the project so far, but we should definitely improve on that!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-141091784
@mjsax test case added
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-139226620
@zentol Any news on this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-139741907
I am not familiar with the TypeExtractor in detail, but would support to fix the bug first and open a separate issue to refactor the extractor, if that is possible.
@StephanEwen, @tillrohrmann What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-134129643
I agree, will get right on it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-140810066
I've removed the refactoring commit and did a rebase.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-135370839
@StephanEwen yes, generally InvalidTypesExceptions are not propagated, except TypeExtractor.getInfoFor(Class ...), which is both public and used in the TypeExtractor.
If an operator that can't tolerate MissingTypeInfo is given one, when will it fail?
There are basically 2 options that i see:
* keep returning a MissingTypeInfo, but go through call to TypeExtractor methods and add a check for MissingTypeInfo if necessary
* throw a checked exception instead of returning a MissingTypeInfo, and go through every call to TypeExtractor methods and add try/catch blocks.
essentially both options are the same, so do we prefer an exception or a MissingTypeInfo?
either way, we're gonna need a lot more changes :/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by mjsax <gi...@git.apache.org>.
Github user mjsax commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-141121393
One failing test in new to me. Do we need a JIRA for it?
```
Tests in error:
CompactingHashTableTest.testHashTableGrowthWithInsert:98->getMemory:243 ยป OutOfMemory
```
The other one, is our good old friend `YARNSessionFIFOITCase`.
I guess it can get merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-135365948
@zentol This is a delicate change, so let's make sure we understand this correctly:
The InvalidTypesException is now only used internally in the type extractor, and never propagated?
I am a bit skeptical about returning the "MissingTypeInfo" in the type extractor directly. A lot of other places / user code classes create type infos and most of them will not check for "MissingTypeInfo" - they rather expect to fail. Only very specific operator places can tolerate the MissingTypeInfo, because they can swap it with a type hint later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-134108201
The PR seems to solve the problem. However, I'm wondering why the `InvalidTypesException` is an unchecked exception. I think that this is the actual culprit and should be fixed. Moreover, according to: Joshua Bloch in "Effective Java"
> Use checked exceptions for recoverable conditions and runtime exceptions for programming errors (Item 58 in 2nd edition)
In our case we can recover from it by returning a `MissingTypeInformation`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2557] TypeExtractor properly returns Mi...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/1045#issuecomment-134203619
InvalidTypesException (from here on abbreviated as ITE) is no longer unchecked.
For this to work i had to make changes in surprisingly many classes so let's break it down:
* In general, InvalidProgramExceptions are now thrown for unrecoverable conditions, where previously an ITE would be thrown..
* TypeExtractor methods that return a TypeInformation will never throw an ITE and instead return a MissingTypeInfo if it is allowed, otherwise an InvalidProgramException.
* One public TypeExtractor method now throws an ITE, which is getParameterType(). This method is heavily used in the TypeExtractor itself as well, so i didn't see a way to fix this in a non API breaking way that doesn't rely yet again on unchecked exceptions.
* affects hadoop-compatibility functions, which now catch them
* A few OperatorClasses catched ITE's in returns(TypeInformation ...) and then manually created a MissingTypeInfo. This now happens in the TypeExtractor directly.
* TypeInformation classes threw an ITE in getInfoFor(Class ...) if the given class didn't match the TypeInformation class. These have been changed to IllegalArgumentExceptions.
* DataSet/DataStream threw an ITE in getType() if the type was a MissingTypeInfo, changed to InvalidProgramException.
* similiar issue in StreamExecutionEnvironment
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---