You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/06/12 16:00:28 UTC
[GitHub] [beam] tysonjh opened a new pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
tysonjh opened a new pull request #12001:
URL: https://github.com/apache/beam/pull/12001
Java 11 removed some Java EE modules. The dependency in MetricsHttpSink
is for HttpException. Replace this instead with an IOException.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Samza | Spark
--- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tysonjh commented on pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
tysonjh commented on pull request #12001:
URL: https://github.com/apache/beam/pull/12001#issuecomment-646766782
> LGTM! Thanks for the changes, that's perfect. Last thing to do is clean review comments: can you squash the first 2 commits and leave only the comment about dependency. The 3rd commit needs to be appart IMHO because it changes the (deprecated) API. Though, reword it to: "remove deprecated MetricName.name() and MetricName.namespace() methods and fix a typo".
> regarding build failure, it's an unrelated timeout, a new build will be triggered when you push.
> Thanks
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on a change in pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
echauchot commented on a change in pull request #12001:
URL: https://github.com/apache/beam/pull/12001#discussion_r440982503
##########
File path: runners/extensions-java/metrics/src/main/java/org/apache/beam/runners/extensions/metrics/MetricsHttpSink.java
##########
@@ -74,7 +74,9 @@ public void writeMetrics(MetricQueryResults metricQueryResults) throws Exception
}
int responseCode = connection.getResponseCode();
if (responseCode != 200) {
- throw new IOException("Expected OK 200 response but received: " + responseCode);
+ throw new IOException(
+ "Expected HTTP 200 OK response while writing metrics to MetricsSinkHttp but received "
Review comment:
sed s/MetricsSinkHttp/MetricsHttpSink/
##########
File path: runners/extensions-java/metrics/src/main/java/org/apache/beam/runners/extensions/metrics/MetricsHttpSink.java
##########
@@ -86,8 +93,8 @@ public MetricNameSerializer(Class<MetricName> t) {
public void serialize(MetricName value, JsonGenerator gen, SerializerProvider provider)
throws IOException {
gen.writeStartObject();
- gen.writeObjectField("name", value.name());
- gen.writeObjectField("namespace", value.namespace());
Review comment:
yes I replaced them with get* versions to simplify serialization code but they were re-introduced to be compatible with dataflow hosted engine which uses .name and .namespace which I could not know of because it is not opensource. Anyway good catch ! Dataflow is supposed to have migrated to .get* versions now, can you check ? If so we can remove the deprecated versions from the api.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tysonjh commented on a change in pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
tysonjh commented on a change in pull request #12001:
URL: https://github.com/apache/beam/pull/12001#discussion_r441874698
##########
File path: runners/extensions-java/metrics/src/main/java/org/apache/beam/runners/extensions/metrics/MetricsHttpSink.java
##########
@@ -74,7 +74,9 @@ public void writeMetrics(MetricQueryResults metricQueryResults) throws Exception
}
int responseCode = connection.getResponseCode();
if (responseCode != 200) {
- throw new IOException("Expected OK 200 response but received: " + responseCode);
+ throw new IOException(
+ "Expected HTTP 200 OK response while writing metrics to MetricsSinkHttp but received "
Review comment:
Oops! Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on a change in pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
echauchot commented on a change in pull request #12001:
URL: https://github.com/apache/beam/pull/12001#discussion_r440838547
##########
File path: runners/extensions-java/metrics/src/main/java/org/apache/beam/runners/extensions/metrics/MetricsHttpSink.java
##########
@@ -69,7 +74,7 @@ public void writeMetrics(MetricQueryResults metricQueryResults) throws Exception
}
int responseCode = connection.getResponseCode();
if (responseCode != 200) {
- throw new HTTPException(responseCode);
+ throw new IOException("Expected OK 200 response but received: " + responseCode);
Review comment:
Agreed as there is no suitable http exception in java.net regular package. But please change the message to add more context "Exception while writing metrics to MetricsHttpSink : expected HTTP 200 OK response but received "
##########
File path: runners/extensions-java/metrics/src/main/java/org/apache/beam/runners/extensions/metrics/MetricsHttpSink.java
##########
@@ -50,6 +49,12 @@ public MetricsHttpSink(MetricsOptions pipelineOptions) {
this.urlString = pipelineOptions.getMetricsHttpSinkUrl();
}
+ /**
+ * Writes the metricQueryResults via HTTP POST to metrics sink endpoint.
+ *
+ * @param metricQueryResults query results to write.
+ * @throws Exception throws IOException for non-200 response from endpoint.
+ */
Review comment:
thanks for adding this missing javadoc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #12001:
URL: https://github.com/apache/beam/pull/12001#issuecomment-647323226
Perfect ! merging
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tysonjh commented on pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
tysonjh commented on pull request #12001:
URL: https://github.com/apache/beam/pull/12001#issuecomment-643353229
R: @echauchot
Are you able to review this PR please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tysonjh commented on a change in pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
tysonjh commented on a change in pull request #12001:
URL: https://github.com/apache/beam/pull/12001#discussion_r440918707
##########
File path: runners/extensions-java/metrics/src/main/java/org/apache/beam/runners/extensions/metrics/MetricsHttpSink.java
##########
@@ -50,6 +49,12 @@ public MetricsHttpSink(MetricsOptions pipelineOptions) {
this.urlString = pipelineOptions.getMetricsHttpSinkUrl();
}
+ /**
+ * Writes the metricQueryResults via HTTP POST to metrics sink endpoint.
+ *
+ * @param metricQueryResults query results to write.
+ * @throws Exception throws IOException for non-200 response from endpoint.
+ */
Review comment:
You're welcome.
##########
File path: runners/extensions-java/metrics/src/main/java/org/apache/beam/runners/extensions/metrics/MetricsHttpSink.java
##########
@@ -86,8 +93,8 @@ public MetricNameSerializer(Class<MetricName> t) {
public void serialize(MetricName value, JsonGenerator gen, SerializerProvider provider)
throws IOException {
gen.writeStartObject();
- gen.writeObjectField("name", value.name());
- gen.writeObjectField("namespace", value.namespace());
Review comment:
These were deprecated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #12001:
URL: https://github.com/apache/beam/pull/12001#issuecomment-644743320
@tysonjh thanks for your work ! Reviewing right now
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tysonjh commented on pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
tysonjh commented on pull request #12001:
URL: https://github.com/apache/beam/pull/12001#issuecomment-644821272
Thanks for the review @echauchot, it is ready for another look.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot merged pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws and remove deprecated MetricName methods
Posted by GitBox <gi...@apache.org>.
echauchot merged pull request #12001:
URL: https://github.com/apache/beam/pull/12001
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] echauchot commented on pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
echauchot commented on pull request #12001:
URL: https://github.com/apache/beam/pull/12001#issuecomment-646537163
LGTM! Thanks for the changes, that's perfect. Last thing to do is clean review comments: can you squash the first 2 commits and leave only the comment about dependency. The 3rd commit needs to be appart IMHO because it changes the (deprecated) API. Though, reword it to: "remove deprecated MetricName.name() and MetricName.namespace() methods and fix a typo".
regarding build failure, it's an unrelated timeout, a new build will be triggered when you push.
Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] apilloud commented on pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
apilloud commented on pull request #12001:
URL: https://github.com/apache/beam/pull/12001#issuecomment-643363003
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tysonjh commented on a change in pull request #12001: [BEAM-10082] Remove dependency on java.xml.ws.
Posted by GitBox <gi...@apache.org>.
tysonjh commented on a change in pull request #12001:
URL: https://github.com/apache/beam/pull/12001#discussion_r441875810
##########
File path: runners/extensions-java/metrics/src/main/java/org/apache/beam/runners/extensions/metrics/MetricsHttpSink.java
##########
@@ -86,8 +93,8 @@ public MetricNameSerializer(Class<MetricName> t) {
public void serialize(MetricName value, JsonGenerator gen, SerializerProvider provider)
throws IOException {
gen.writeStartObject();
- gen.writeObjectField("name", value.name());
- gen.writeObjectField("namespace", value.namespace());
Review comment:
Got it, I was just doing some code cleaning as I went along. I've verified that removing the deprecated methods doesn't break anything and added that to this PR. I'll update the commit message as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org