Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature][Zeta] Improve local mode startup request ports #4660

Merged
merged 3 commits into from
May 16, 2023

Conversation

liunaijie
Copy link
Contributor

Purpose of this pull request

close #4607
tested with this command:

./bin/seatunnel.sh --config ./config/v2.batch.config.template -e local

the result is:
image
image

this is same pr with this. I had some missteps that caused the previous PR to be closed.

Check list

@liunaijie liunaijie changed the title [Feature][Zeta] Improve local mode startup request ports #4607 [Feature][Zeta] Improve local mode startup request ports Apr 24, 2023
@liunaijie
Copy link
Contributor Author

also tested with the SeaTunnelEngineExample i am not sure why there has some action failed.

image

@liunaijie
Copy link
Contributor Author

@hailin0 PTAL

@EricJoy2048
Copy link
Member

Can you add test case for this change? Like use e2e create two local mode job in one container at same time.

@hailin0
Copy link
Contributor

hailin0 commented Apr 28, 2023

Can you add test case for this change? Like use e2e create two local mode job in one container at same time.

+1

@liunaijie
Copy link
Contributor Author

this looks a bug

engine-v2-it failed due to ClusterFaultToleranceTwoPipelineIT.testTwoPipelineStreamJobRestoreIn2NodeMasterDown:732 » ConditionTimeout

I checked this method, this method crate 2 hazelcast serve instance, and shutdown 1 instance when some tasks commit finished.
when the left node became master node, and start recover job, get NPE, zeta canceled the job, so the check won't be success. when wait time is over, failed.

Please see the screenshot i attached.

error log

image

the left node change to master

image

get NPE

image

task canceled

image

@liunaijie
Copy link
Contributor Author

retest

@liunaijie
Copy link
Contributor Author

@hailin0 please rerun the actions
in the previous failed actions, i find zeta engine has an bug. when restore task, there has somewhere past a NULL to SeaTunnelTask.restoreState method, in this method doesn't have parameter check, so get NPE and cause cancel the job. so i added a parameter check code.
Also i think we also need to find the root cause, where generated a NULL and past to this method.

@EricJoy2048
Copy link
Member

Please fix the code style error.

@liunaijie
Copy link
Contributor Author

Please fix the code style error.

done

@liunaijie
Copy link
Contributor Author

@EricJoy2048 @hailin0 . now this config has error. the assert sink has a min value check but the fake source doesn't has the min value config.

For Now, this pr has 3 commit:

  1. for issue [Feature][Zeta] Improve local mode startup request ports  #4607 , improve local mode startup request ports
  2. fix zeta engine recover job get NPE. (but it's not the finially solution, just added parameter check, need find where past the NULL to this method)
  3. fix Assert e2e config error

@hailin0
Copy link
Contributor

hailin0 commented May 6, 2023

@EricJoy2048 @hailin0 . now this config has error. the assert sink has a min value check but the fake source doesn't has the min value config.

For Now, this pr has 3 commit:

  1. for issue [Feature][Zeta] Improve local mode startup request ports  #4607 , improve local mode startup request ports
  2. fix zeta engine recover job get NPE. (but it's not the finially solution, just added parameter check, need find where past the NULL to this method)
  3. fix Assert e2e config error
  1. suggestion remove MIN check from assert sink

@liunaijie
Copy link
Contributor Author

@EricJoy2048 @hailin0 . now this config has error. the assert sink has a min value check but the fake source doesn't has the min value config.
For Now, this pr has 3 commit:

  1. for issue [Feature][Zeta] Improve local mode startup request ports  #4607 , improve local mode startup request ports
  2. fix zeta engine recover job get NPE. (but it's not the finially solution, just added parameter check, need find where past the NULL to this method)
  3. fix Assert e2e config error
  1. suggestion remove MIN check from assert sink

I think in the assert e2e check, this is a good validaction rule, we just need update the source config.

@hailin0
Copy link
Contributor

hailin0 commented May 6, 2023

@EricJoy2048 @hailin0 . now this config has error. the assert sink has a min value check but the fake source doesn't has the min value config.
For Now, this pr has 3 commit:

  1. for issue [Feature][Zeta] Improve local mode startup request ports  #4607 , improve local mode startup request ports
  2. fix zeta engine recover job get NPE. (but it's not the finially solution, just added parameter check, need find where past the NULL to this method)
  3. fix Assert e2e config error
  1. suggestion remove MIN check from assert sink

I think in the assert e2e check, this is a good validaction rule, we just need update the source config.

agree

@hailin0
Copy link
Contributor

hailin0 commented May 6, 2023

@EricJoy2048 @hailin0 . now this config has error. the assert sink has a min value check but the fake source doesn't has the min value config.

For Now, this pr has 3 commit:

  1. for issue [Feature][Zeta] Improve local mode startup request ports  #4607 , improve local mode startup request ports
  2. fix zeta engine recover job get NPE. (but it's not the finially solution, just added parameter check, need find where past the NULL to this method)
  3. fix Assert e2e config error

@EricJoy2048
What is your opinion on question 2

@EricJoy2048
Copy link
Member

@EricJoy2048 @hailin0 . now this config has error. the assert sink has a min value check but the fake source doesn't has the min value config.
For Now, this pr has 3 commit:

  1. for issue [Feature][Zeta] Improve local mode startup request ports  #4607 , improve local mode startup request ports
  2. fix zeta engine recover job get NPE. (but it's not the finially solution, just added parameter check, need find where past the NULL to this method)
  3. fix Assert e2e config error

@EricJoy2048 What is your opinion on question 2

It's ok.

@liunaijie
Copy link
Contributor Author

@hailin0 please help review and merge this pr, thanks.

@Hisoka-X Hisoka-X added this to the 2.3.2 milestone May 12, 2023
@EricJoy2048 EricJoy2048 merged commit 2050bf5 into apache:dev May 16, 2023
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][Zeta] Improve local mode startup request ports
4 participants