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

[tillrohrmann]

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...

[yanghua]

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...

[tillrohrmann]

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  wrote:

> 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?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> , or 
mute
> the thread
> 

> .
>



---

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

[yanghua]

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...

[zentol]

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...

[GJL]

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...

[zentol]

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...

[yanghua]

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...

[yanghua]

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...

[zentol]

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...

[yanghua]

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...

[GJL]

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...

[yanghua]

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...

[GJL]

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 `true`, then 
`SerializedValue` should be part of the JSON response (in addition to the 
stringified value), otherwise only the stringified value. By default 
`includeSerializedValue` should be `false` because the Web UI cannot handle 
binary data.  For the request in `RestClusterClient` you would always set the 
flag to `true`.

Let me know if you have any questions.

cc: @tillrohrmann 






---

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

[yanghua]

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 `getAccumulatorsSerialized ` , so I 
took steps along this path and split the stringified and serialized 
accumulators.

So the conclusion is : accepting your opinion and taking the each key's 
SerializedValue into `UserTaskAccumulator` ? 

If yes, I will try to change the code.


---

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

[GJL]

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`, what would speak against adding a 
new field to `UserTaskAccumulator` which stores the `SerializedValue`? The JSON 
representation would always carry an additional base64 encoded `byte` array but 
I think performance isn't important at this point.

cc: @tillrohrmann @aljoscha 


---

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

[aljoscha]

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 `ClusterClient`: it retrieves the 
serialised values and returns a map of the deserialised values.


---

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

[yanghua]

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 contains to two representation mode 
(***stringified***  and ***serialized***)

* define a `JobAccumulatorsMessageParameters` class which extends 
`MessageParameters`, and the `getQueryParameters` method will return the 
`AccumulatorRepresentationsQueryParameter`

* refactor `JobAccumulatorsHeaders#getUnresolvedMessageParameters` return 
`JobAccumulatorsMessageParameters`'s instance

* refactor `JobAccumulatorsHandler#handleRequest` it will query specific 
accumulator's representation base of 
`AccumulatorRepresentationsQueryParameter`, and the `JobAccumulatorsInfo` will 
be reused for both representations

* in `RestClusterClient` class , let `getAccumulators` return stringified 
accumulators(`Map`) and `getSerializedAccumulators` return 
serialized accumulators(`Map`)

@tillrohrmann and @GJL  hope for your opinions, Thanks.



---

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

[aljoscha]

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 serialized accumulators. For now, 
the former would be used for the web frontend while the latter would be used by 
the `RestClusterClient`. @GJL maybe has a better opinion on how this should be 
done on the server side, though.


---

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

[yanghua]

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?

In `JobAccumulatorsHandler#handleRequest` method, we could query the 
accumulator's string and serialized representations, and boxed in 
`JobAccumulatorsInfo` object. 

1. let `getAccumulators` return accumulator's string representations and 
`getSerializedAccumulators` method return accumulator's serialized 
representations.

2.  let `getAccumulators` return both representations?

Which one is your idea?


---

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

[aljoscha]

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 we keep both versions.


---

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

[yanghua]

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 serialized accumulators in `ClusterClient`. And override 
`getSerializedAccumulators` in `RestClusterClient`. For `JobAccumulatorsInfo`, 
I also remained `UserTaskAccumulator` and just added a new property for 
serialized accumulators. Please review, thanks.


---

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

[GJL]

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 class 
for ser/des.


---

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

[yanghua]

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 getter and setter

and the two ways both failed with different reason: 

* first way : throw a exception : `SerializedValue` no suitable constructor;
* second way : the setted object will be ignore by 
`HandlerUtils#sendResponse`

In `JobAccumulatorsHandler#handleRequest` I can not return `Map` directly, because this method's result type must 
extends `ResponseBody`.


---

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

[yanghua]

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 
[FLINK-8459](https://github.com/apache/flink/pull/5565)? Thanks!


---

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

[tillrohrmann]

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 have to adapt the web ui to work 
with this format.


---

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

[aljoscha]

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 for accumulators does, to 
allow user defined accumulator types.


---

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

[yanghua]

Github user yanghua commented on the issue:

https://github.com/apache/flink/pull/5573
  
HI @tillrohrmann and @aljoscha , who would review this PR? Thanks!


---