You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by fpompermaier <gi...@git.apache.org> on 2016/05/19 09:42:34 UTC
[GitHub] flink pull request: [Flink-3908] Fixed Parser's error state reset
GitHub user fpompermaier opened a pull request:
https://github.com/apache/flink/pull/2007
[Flink-3908] Fixed Parser's error state reset
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [X] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [X] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [X] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/fpompermaier/flink FLINK-3908
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2007.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 #2007
----
commit 25795388a7585dc100470e9cb5bcf89833ea3a03
Author: Flavio Pompermaier <f....@gmail.com>
Date: 2016-05-19T09:41:43Z
[Flink-3908] Fixed Parser's error state reset
----
---
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 issue #2007: [FLINK-3908] Fixed Parser's error state reset
Posted by fpompermaier <gi...@git.apache.org>.
Github user fpompermaier commented on the issue:
https://github.com/apache/flink/pull/2007
I think that leaving the responsibility of calling resetErrorStateAndParse() to "consumers" of FieldParser is really a bad idea. Isn't safer to force its call using the strategy adopted in this PR (i.e. keep the real implementation in parseFieldImpl and use parseField to call resetErrorStateAndParse before 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 issue #2007: [FLINK-3908] Fixed Parser's error state reset
Posted by fpompermaier <gi...@git.apache.org>.
Github user fpompermaier commented on the issue:
https://github.com/apache/flink/pull/2007
Now it should be ok, according to your suggestions. I misunderstood what @StephanEwen was trying to say, thanks @zentol for the clarification!
Just another thing: the method GenericCsvInputFormat.checkAndCoSort() is never used in the code. Do you want to keep 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-3908] Fixed Parser's error state reset
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/2007
i believe what Stephan meant was the following:
* keep the abstract parseField method in FieldParser
* add a new method to FieldParser called resetErrorStateAndParse, which calls resetParserState and parseField
This would require reverting all changes you made to specific parsers (like LongValueParser), and modifiying each call to parseField from outside the parser, like the GenericCsvInputFormat.
---
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 issue #2007: [FLINK-3908] Fixed Parser's error state reset
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2007
I'm not sure; in any case it should not be removed as part of this PR.
You can open a separate JIRA or ask on the mailing list.
---
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-3908] Fixed Parser's error state reset
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/2007#issuecomment-221876052
I think I meant it a bit differently than I guess you understood it.
It is basically the exact same implementation you have now, only with different method names.
---
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 issue #2007: [FLINK-3908] Fixed Parser's error state reset
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/2007
Merging
---
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-3908] Fixed Parser's error state reset
Posted by fpompermaier <gi...@git.apache.org>.
Github user fpompermaier commented on the pull request:
https://github.com/apache/flink/pull/2007#issuecomment-221862570
Hi Stephan, thanks for reviewing this PR.
These are my reasons to not apply your suggestions to my PR, and I'd like to hear your opinion (anyone interested by this discussion is also welcome) before reissuing the PR with such modifications:
- rename `reset` to `resetErrorStateAndParse`: for almost all parsers this doesn't do anything more than those 2 operations but what if for a further parserImpl you need to do anything else before parsing?
- resetting the error state is something that must always be done, by all parser implementations. The `parseFieldImpl` is a way to force this thing, otherwise you should rely on classes using such parser to remember this thing (as you suggested with "GenericCsvInputCormat would call the resetErrorStateAndParse") or ensure that every parser call it at the very beginning of the `parseField()`.
I'm really concerned about this. In this way, my PR doesn't break the current APIs
Does it makes sense?
---
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 #2007: [FLINK-3908] Fixed Parser's error state reset
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2007
---
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 issue #2007: [FLINK-3908] Fixed Parser's error state reset
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/2007
Good to merge.
---
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-3908] Fixed Parser's error state reset
Posted by fpompermaier <gi...@git.apache.org>.
Github user fpompermaier commented on the pull request:
https://github.com/apache/flink/pull/2007
How should I proceed here?
---
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-3908] Fixed Parser's error state reset
Posted by fpompermaier <gi...@git.apache.org>.
Github user fpompermaier commented on the pull request:
https://github.com/apache/flink/pull/2007#issuecomment-221880071
I don't understand :(
Assuming that `reset` could be renamed as `resetErrorStateAndParse`, the other 2 suggestions cannot be applied to my current implementation: if I want to rename `parseFieldImpl` to `parseField` I should overload somehow the method otherwise that's not possible because there's already an abstract method called parseField with the same sign..
Wrt leaving the responsibility to reset the state of the parsers to the classes calling the parseField is quite dangerous IMHo (as you said in "GenericCsvInputCormat would call the resetErrorStateAndParse").
Am I misunderstanding something?
---
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 issue #2007: [FLINK-3908] Fixed Parser's error state reset
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2007
then make parseField protected so users MUST call resetErrorStateAndParse.
there, problem solved.
---
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-3908] Fixed Parser's error state reset
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/2007#issuecomment-221858427
Thanks for addressing this bug!
How about slightly changing the names of the methods:
- The method that resets the error state and parses would be called `resetErrorStateAndParse(...)`. That would make it clear what happens.
- The `parseFieldImpl(...)` could be kept as `parseField(...)`
- The `GenericCsvInputCormat` would call the `resetErrorStateAndParse(...)` method.
Other than that, solution looks good!
---
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.
---