[
https://issues.apache.org/jira/browse/BEAM-5141?focusedWorklogId=134377&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-134377
]
ASF GitHub Bot logged work on BEAM-5141:
----------------------------------------
Author: ASF GitHub Bot
Created on: 14/Aug/18 01:01
Start Date: 14/Aug/18 01:01
Worklog Time Spent: 10m
Work Description: akedin commented on a change in pull request #6216:
[BEAM-5141] Improve error message when SET unregistered options
URL: https://github.com/apache/beam/pull/6216#discussion_r209803149
##########
File path:
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamEnumerableConverter.java
##########
@@ -118,7 +120,25 @@ public static PipelineOptions
createPipelineOptions(Map<String, String> map) {
for (Map.Entry<String, String> entry : map.entrySet()) {
args[i++] = "--" + entry.getKey() + "=" + entry.getValue();
}
- PipelineOptions options =
PipelineOptionsFactory.fromArgs(args).withValidation().create();
+
+ PipelineOptions options = null;
+ try {
+ options =
PipelineOptionsFactory.fromArgs(args).withValidation().create();
+ } catch (IllegalArgumentException e) {
+ Matcher matcher =
+ Pattern.compile("Class (.+) missing a property named
\\'(.+)\\'\\.(.*)")
+ .matcher(e.getMessage());
+ if (matcher.matches()) {
Review comment:
I think it's not a good thing to catch exceptions by string matching, and
then parsing. It's brittle, it's slow, it's error-prone.
It's a good practice to create your own exception wrapper type specific for
each context exactly for this reason. I don't know if there's a specific reason
why `PipelineOptionsFactory` throws `IllegalArgumentException` instead of
creating a separate exception type but I think that it's better to change this,
e.g. create a new class `PipelineOptionsUnexpectedPropertyException extends
IllegalArgumentException` with fields like `option`, `bestMatch`, `className`.
Then when wrapping and rethrowing it you would have all the information needed.
My guess is that `PipelineOptionsFactory` is usually very close to the top
of the call stack so there's not a lot of need to re-interpret the exception,
and `IllegalArgumentException` is enough for most cases. But we wrap the whole
pipeline business into a DSL, so it is helpful to re-state the error with more
context
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 134377)
Time Spent: 1h 20m (was: 1h 10m)
> Improve error message when SET unregistered options
> ----------------------------------------------------
>
> Key: BEAM-5141
> URL: https://issues.apache.org/jira/browse/BEAM-5141
> Project: Beam
> Issue Type: Improvement
> Components: dsl-sql
> Reporter: Rui Wang
> Assignee: Rui Wang
> Priority: Major
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
