[GitHub] flink pull request: [FLINK-3908] Fixed Parser's error state reset
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3908] Fixed Parser's error state reset
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3908] Fixed Parser's error state reset
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3908] Fixed Parser's error state reset
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3908] Fixed Parser's error state reset
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-3908] Fixed Parser's error state reset
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[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
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
