Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/3943
Merging...
---
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
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
Hi @twalthr @fhueske thanks a lot for your review.
@twalthr your improve changes are make the structure of the package is
clearer which make sense to me.
+1 to merge.
---
If
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3943
Hi @twalthr and @sunjincheng121, I looked over the package structure and
naming of the tests and this looks good to me.
+1 to merge from my side.
---
If your project is set up for it, you
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/3943
@wuchong @sunjincheng121 @fhueske
I tried to simplify the package structure a little bit. I based my changes
on top of this PRs commit.
My goals:
- splitting a class like
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
Rebase code according master.
---
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
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
Thanks @twalthr ! I have rebased the code and updated the PR.
Thanks,
SunJincheng
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/3943
Thanks for the update @sunjincheng121. I will go over all changes and try
to merge this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
Update again. Because the new merged code.
---
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
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
@twalthr I have rebase the PR. I appreciated if you can review it. And i'll
quick updated it when you left comments.
Thanks,
SunJincheng
---
If your project is set up for it,
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
@twalthr Extremely grateful If you can review and merge this PR after I
rebase the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3943
@twalthr, sure! Thanks for taking care of this PR.
---
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
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/3943
Thanks @twalthr , I'm fine with this ,a great +1!
---
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
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/3943
@sunjincheng121 the structure looks very good. I would volunteer to go over
the changes as a whole and merge this. If @fhueske @wuchong are fine with this?
Can you rebase the PR a last time?
---
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3943
I think that is a very good idea. A review of a +6k -4k LOC PR takes a lot
of time and has a good chance to be interrupted by other issues. Several
smaller PRs would be much easier to review and
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
May be I need split this PR into a couple of small PRs. What do you think?
or have any other suggestions @fhueske @wuchong @twalthr
---
If your project is set up for it, you can reply to
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
Rebase code...
---
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
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
Hi @wuchong Thanks for your approval.
In addition to refactoring the test structure, I think that in addition to
ITCast other tests should be unified use of `batchTestUtil` and`
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/3943
Hi @sunjincheng121 , thanks for your great work. I like your proposal. The
test structure refactoring looks good to me. I think it's good to put all the
IT cases in one place.
---
If your project
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
**I once again seriously consider the Refactoring work, according to the
following principles updated the PR.**
  **First, the test type and purpose(4 types):**
* Unit Test:
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
@twalthr I have rebased code against master.
Best,
SunJincheng
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3943
Hi @twalthr Thanks a log for your reviewing. I completely agree with your
suggestions. I petty want refactoring the test structure. I have updated the
PR. according your comments.
21 matches
Mail list logo
