[GitHub] flink pull request #6183: [FLINK-9616][metrics] Fix datadog to include shade...
Github user addisonj closed the pull request at: https://github.com/apache/flink/pull/6183 ---
[GitHub] flink issue #6183: [FLINK-9616][metrics] Fix datadog to include shaded deps
Github user addisonj commented on the issue: https://github.com/apache/flink/pull/6183 This is fixed by #6191 ---
[GitHub] flink pull request #6183: [FLINK-9616][metrics] Fix datadog to include shade...
GitHub user addisonj opened a pull request: https://github.com/apache/flink/pull/6183 [FLINK-9616][metrics] Fix datadog to include shaded deps ## What is the purpose of the change This fixes a broken build that wasn't properly including a shaded in the jar it builds. This causes the instantiation of DatadogHttpReporter to fail and with no easy way to fix since the dependencies exist on a shaded import path. ## Brief change log - Changes ## Verifying this change This change is a trivial rework / code cleanup without any test coverage. However, it can be validated by: ``` cd flink-metrics/flink-metrics-datadog mvn package jar tf target/flink-metrics-datadog-1.6-SNAPSHOT.jar ``` And then seeing the expected okhttp3 and okio dependencies being included in the resulting jar. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): yes, but brings in line with documented behavior here: https://ci.apache.org/projects/flink/flink-docs-release-1.5/monitoring/metrics.html#datadog-orgapacheflinkmetricsdatadogdatadoghttpreporter - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no - The serializers: no - The runtime per-record code paths (performance sensitive): no - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no - The S3 file system connector: no ## Documentation - Does this pull request introduce a new feature? no - If yes, how is the feature documented? not applicable You can merge this pull request into a Git repository by running: $ git pull https://github.com/instructure/flink datadog_fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/6183.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 #6183 commit 10fe56a9adbe6f35dc4b5fae0e7478e99028f5f7 Author: Addison Higham Date: 2018-06-19T14:49:56Z [FLINK-9616] Fix datadog to include shaded deps flink-metrics-datadog wasn't properly included it's shaded dependencies in the jar it builds. Looking at other places where shaded dependecies are used, it seems like the build wasn't working as intended. ---
[GitHub] flink issue #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on the issue: https://github.com/apache/flink/pull/3481 @tillrohrmann this was the PR I mentioned to you during flink foward, if you get a chance to look :) --- 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 issue #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on the issue: https://github.com/apache/flink/pull/3481 @EronWright good suggestion in regards to containerInfo without an image name, I confirmed in the mesos docs that it should work that way. Lemme know if there is anything else! --- 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 #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on a diff in the pull request:
https://github.com/apache/flink/pull/3481#discussion_r108790880
--- Diff:
flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/MesosTaskManagerParameters.java
---
@@ -162,11 +182,65 @@ public static MesosTaskManagerParameters
create(Configuration flinkConfig) {
throw new
IllegalConfigurationException("invalid container type: " + containerTypeString);
}
+ Option containerVolOpt =
Option.apply(flinkConfig.getString(MESOS_RM_CONTAINER_VOLUMES));
+ List containerVolumes =
buildVolumes(containerVolOpt);
+
return new MesosTaskManagerParameters(
cpus,
containerType,
Option.apply(imageName),
- containeredParameters);
+ containeredParameters,
+ containerVolumes);
+ }
+
+ /**
+* Used to build volume specs for mesos. This allows for mounting
additional volumes into a container
+*
+* @param containerVolumes a comma delimited optional string of
[host_path:]container_path[:RO|RW] that
+* defines mount points for a container volume.
If None or empty string, returns
+* an empty iterator
+*/
+ public static List buildVolumes(Option
containerVolumes) {
+ if (containerVolumes.isEmpty()) {
+ return new ArrayList();
+ }
+ String[] specs = containerVolumes.get().split(",");
+ List vols = new ArrayList();
+ for (String s : specs) {
+ if (s.trim().isEmpty()) {
+ continue;
+ }
+ Protos.Volume.Builder vol = Protos.Volume.newBuilder();
+ vol.setMode(Protos.Volume.Mode.RW);
+
+ String[] parts = s.split(":");
+ switch (parts.length) {
+ case 1:
+ vol.setContainerPath(parts[0]);
+ break;
+ case 2:
+ try {
+ Protos.Volume.Mode mode =
Protos.Volume.Mode.valueOf(parts[1].trim().toUpperCase());
+ vol.setMode(mode)
+
.setContainerPath(parts[0]);
+ } catch (IllegalArgumentException e) {
--- End diff --
totally agree, it is strange. But this is the same spec that docker CLI
uses as well as the spark mesos framework... It doesn't seem ideal and is
definitely somewhat of a sharp edge but seemed best to just use the same
standard
---
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 issue #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on the issue: https://github.com/apache/flink/pull/3481 @EronWright @zentol minor bump on this... any other steps to get this on the path to being merged? Don't want to let this hang out for too long so I forgot about 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #1668: [FLINK-3257] Add Exactly-Once Processing Guarantees for I...
Github user addisonj commented on the issue: https://github.com/apache/flink/pull/1668 Very interested in this work. It sounds like there are few loose ends and then some cleanup before it might be ready for merge, @senorcarbone or @StephanEwen anything that can be supported by someone else? Would love to help wherever possible --- 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 issue #3344: FLINK-5731 Spilt up tests into three disjoint groups
Github user addisonj commented on the issue: https://github.com/apache/flink/pull/3344 Even with this in place, I am still regularly seeing timeouts on quite a few PRs. In addition to the travis queue being backed up, its taking 12+ hours to get feedback which is mostly just build timeouts :| --- 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 issue #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on the issue: https://github.com/apache/flink/pull/3481 Okay, just moved things around, agree that its a better place, but was hesitant at first as we were building any other mesos objects earlier. Hopefully this still works. I think going forwad and adding support for other container options, such as additional network ports, additional URIs to download, that would be a better place to put them. Also, I have one other small commit, it wasn't because the volume info wasn't getting attached soon enough. This now is working for me in my dev env, which is 1.2.0 with these patches applied --- 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 #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on a diff in the pull request:
https://github.com/apache/flink/pull/3481#discussion_r104838693
--- Diff:
flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
---
@@ -262,9 +264,61 @@ public String toString() {
taskInfo.setContainer(containerInfo);
}
+
containerInfo.addAllVolumes(buildVolumes(params.containerVolumes()));
+
return taskInfo.build();
}
+ /**
+* Used to build volume specs for mesos. This allows for mounting
additional volumes into a container
+*
+* @param containerVolumes a comma delimited optional string of
[host_path:]container_path[:RO|RW] that
+* defines mount points for a container volume.
If None or empty string, returns
+* an empty iterator
+*/
+ public static List buildVolumes(Option
containerVolumes) {
+ if (containerVolumes.isEmpty()) {
+ return new ArrayList();
+ }
+ String[] specs = containerVolumes.get().split(",");
+ List vols = new ArrayList();
+ for (String s : specs) {
+ if (s.trim().isEmpty()) {
+ continue;
+ }
+ Protos.Volume.Builder vol = Protos.Volume.newBuilder();
+ vol.setMode(Protos.Volume.Mode.RW);
+
+ String[] parts = s.split(":");
+ switch (parts.length) {
+ case 1:
+ vol.setContainerPath(parts[0]);
+ break;
+ case 2:
+ try {
+ Protos.Volume.Mode mode =
Protos.Volume.Mode.valueOf(parts[1].trim().toUpperCase());
+ vol.setMode(mode)
+
.setContainerPath(parts[0]);
+ } catch (IllegalArgumentException e) {
+ vol.setHostPath(parts[0])
+
.setContainerPath(parts[1]);
+ }
+ break;
+ case 3:
+ Protos.Volume.Mode mode =
Protos.Volume.Mode.valueOf(parts[2].trim().toUpperCase());
+ vol.setMode(mode)
+ .setHostPath(parts[0])
+ .setContainerPath(parts[1]);
+ break;
+ default:
+ throw new
IllegalArgumentException("volume specification is invalid, given: " + s);
--- End diff --
ð will move it there
---
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 issue #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on the issue: https://github.com/apache/flink/pull/3481 @zentol thanks for the quality reviews. The code reads a lot cleaner with the mode parsing being handled by the enum. My java is pretty rusty after all being in scala of late :) I think that handles everything. Hopefully that makes sense for reasoning of not using a regex. --- 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 #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on a diff in the pull request:
https://github.com/apache/flink/pull/3481#discussion_r104790634
--- Diff:
flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
---
@@ -262,9 +265,67 @@ public String toString() {
taskInfo.setContainer(containerInfo);
}
+ containerInfo.addAllVolumes(volumes(params.containerVolumes()));
+
return taskInfo.build();
}
+ /**
+* Used to build volume specs for mesos. This allows for mounting
additional volumes into a container
+*
+* @param containerVolumes a comma delimited optional string of
[host_path:]container_path[:RO|RW] that
+* defines mount points for a container volume.
If None or empty string, returns
+* an empty iterator
+*/
+ public List volumes(Option containerVolumes) {
+ if (containerVolumes.isEmpty()) {
+ return new ArrayList();
+ }
+ String[] specs = containerVolumes.get().split(",");
+ List vols = new ArrayList();
+ for (int i = 0; i < specs.length; i++) {
+ String s = specs[i];
+ if (s.isEmpty()) {
+ continue;
+ }
+ Protos.Volume.Builder vol = Protos.Volume.newBuilder();
+ vol.setMode(Protos.Volume.Mode.RW);
--- End diff --
correct,
http://mesos.apache.org/api/latest/java/org/apache/mesos/Protos.Volume.Builder.html#setMode(org.apache.mesos.Protos.Volume.Mode).
RW seems like a sane default
---
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 #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on a diff in the pull request:
https://github.com/apache/flink/pull/3481#discussion_r104790136
--- Diff:
flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
---
@@ -262,9 +265,67 @@ public String toString() {
taskInfo.setContainer(containerInfo);
}
+ containerInfo.addAllVolumes(volumes(params.containerVolumes()));
+
return taskInfo.build();
}
+ /**
+* Used to build volume specs for mesos. This allows for mounting
additional volumes into a container
+*
+* @param containerVolumes a comma delimited optional string of
[host_path:]container_path[:RO|RW] that
+* defines mount points for a container volume.
If None or empty string, returns
+* an empty iterator
+*/
+ public List volumes(Option containerVolumes) {
+ if (containerVolumes.isEmpty()) {
+ return new ArrayList();
+ }
+ String[] specs = containerVolumes.get().split(",");
+ List vols = new ArrayList();
+ for (int i = 0; i < specs.length; i++) {
--- End diff --
scala has ruined me... I
---
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 issue #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on the issue: https://github.com/apache/flink/pull/3481 @zentol thanks for the review! I think I addressed all those, updated via an amend. Wasn't sure if you want changes as new commits or not. Also, the build passed locally for me, but I haven't seen it work via travis yet, keeps failing in flink core for what appears to be totally unrelated (guessing flaky tests?). Let me know if there is anything else! --- 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 #3481: [FLINK-5975] Add volume support to flink-mesos
Github user addisonj commented on a diff in the pull request:
https://github.com/apache/flink/pull/3481#discussion_r104752423
--- Diff:
flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
---
@@ -131,9 +134,7 @@ public int getPorts() {
}
@Override
- public List getHardConstraints()
{
- return null;
- }
+ public List getHardConstraints()
{ return null; }
--- End diff --
oops, intellij tricking me
---
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 #3481: [FLINK-5975] Add volume support to flink-mesos
GitHub user addisonj opened a pull request:
https://github.com/apache/flink/pull/3481
[FLINK-5975] Add volume support to flink-mesos
When using containerization, specifically, docker, it is useful to be
able to attach additional volumes, such as an NFS share.
This adds support for volumes to be attached via specifying a new config
values `mesos.resourcemanager.tasks.container.volumes`. This is comma
delimited string of `[host_path:]container_path[:RO|RW]`.
It is modeled after the spark mesos framework
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.
- [ ] 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)
- [ ] 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
- [ ] 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/addisonj/flink master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3481.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 #3481
commit ddce9d20b0dd7a9dd9d5ea0a8cfb5ed61d5685c2
Author: Addison Higham
Date: 2017-03-07T06:40:16Z
[FLINK-5975] Add volume support to flink-mesos
When using containerization, specifically, docker, it is useful to be
able to attach additional volumes, such as an NFS share.
This adds support for volumes to be attached via specifying a new config
values `mesos.resourcemanager.tasks.container.volumes`. This is comma
delimited string of `[host_path:]container_path[:RO|RW]`.
It is modeled after the spark mesos framework
---
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.
---
