[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/5573 Changes look good @yanghua. Merging this PR. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 @tillrohrmann thanks for your reply, it's a misoperation, I have done rollback and refactored the code based on your suggestion. Please check the change again. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/5573 Not sure what exactly happened but you can always go back to an earlier point via the reflog and then a git hard reset. On Tue, Mar 20, 2018 at 4:11 PM, vinoyang

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 hi @tillrohrmann it seems I make a wrong git operation, I merged some new commits and squashed into one, then pushed into the PR(branch). What should I do to fix this problem? ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user zentol commented on the issue: https://github.com/apache/flink/pull/5573 please do not trigger builds just to get that perfect green build. The test failure here is quite common at the moment. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user GJL commented on the issue: https://github.com/apache/flink/pull/5573 You can use `git commit -m "Trigger build" --allow-empty` to trigger a build. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user zentol commented on the issue: https://github.com/apache/flink/pull/5573 @yanghua This test failure is unrelated, you can ignore it. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 @zentol it seems the Travis CI has some problem, always build failed. Please review my latest change. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 hi @zentol , I refactor the code based on your review suggestion. Would you please review again, thanks! ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user zentol commented on the issue: https://github.com/apache/flink/pull/5573 We may want to delay merging this until [FLINK-8881](https://issues.apache.org/jira/browse/FLINK-8881) has been addressed, as it voids the primary use-case of this handler. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 @tillrohrmann , if you have time could you please review this PR? Thanks. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user GJL commented on the issue: https://github.com/apache/flink/pull/5573 @yanghua I will take a look this week. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 @GJL I have refactored the code and test case, could you please review the new commit? Thanks. ---

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user GJL commented on the issue: https://github.com/apache/flink/pull/5573 @yanghua I talked to @tillrohrmann offline, and we decided it is enough to add a query parameter such as `includeSerializedValue` to the `JobAccumulatorsHandler`. If `includeSerializedValue` is

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 @GJL , the before discussion between I and @aljoscha treat the accumulators as a whole (maybe only me?). Because the pre-existing implementation `getAccumulatorResultsStringified ` and

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user GJL commented on the issue: https://github.com/apache/flink/pull/5573 I agree with @aljoscha. We should not add more `public` methods to `ClusterClient`. Implementing `getAccumulators()` in `RestClusterClient` is enough. Regarding the `JobAccumulatorsHandler`,

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5573 I can't comment on the first points but on the last point. I would suggest to only add `getAccumulators()` to `RestClusterClient` that behaves the same way as the existing `getAccumulators()` on

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 OK, base of @aljoscha 's opinion, I describe the implementation detail before coding : * define a `AccumulatorRepresentationsQueryParameter` class which extends `MessageQueryParameter` and

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5573 @yanghua I mean having two separate ways of querying: the server should have two endpoints (or differentiate based on a query parameter) and return either the stringified accumulators or the

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 @aljoscha yes, I agree. > I think it would be good to be able to **query** both representations The "query" you mean `JobAccumulatorsHandler` or `RestClusterClient` behavior?

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5573 I think it would be good to be able to query both representations. The stringified version can be good if you want to do a quick query using a rest client. And we don't have to change the Web UI if

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 @GJL thanks for your guidance! @aljoscha and @tillrohrmann I have refactored this issue, remained the `getAccumulators` method created a `getSerializedAccumulators` method to return the

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user GJL commented on the issue: https://github.com/apache/flink/pull/5573 @yanghua Take a look at `SerializedValueDeserializer` and `SerializedValueSerializer`. You can use `@JsonSerialize(using= ... )`, `@JsonSerialize(keyUsing= ... )`, and `@JsonDeserialize` to specify the

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 Hi @aljoscha and @tillrohrmann I have tried to put `Map` into `JobAccumulatorsInfo` with two ways : * a `@JsonProperty` * marked with `@JsonIgnore` and provide

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 @aljoscha and @tillrohrmann , Thanks for your suggestion. I will rework for this. Moreover, @tillrohrmann would please review my another PR

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/5573 I agree with @aljoscha. We could change the `JobAccumulatorsHandler` to return the serialized representation of the accumulator map instead of the stringified representation. We would then also

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5573 @yanghua I'll have a look next week. From a first look it seems like we might have to extend the rest handler and the client to also return the accumulators as a `SerializedValue<>` as the old code

[GitHub] flink issue #5573: [FLINK-8756][Client] Support ClusterClient.getAccumulator...

Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5573 HI @tillrohrmann and @aljoscha , who would review this PR? Thanks! ---