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.

Sign up for GitHub

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

Jump to bottom

Return app submission time for batch #4119

Closed
wants to merge 8 commits into from
Closed

Return app submission time for batch #4119

wants to merge 8 commits into from

Conversation

turboFei
Copy link
Member

@turboFei turboFei commented Jan 7, 2023 •
edited
Loading

Why are the changes needed?

Return the batch app submission time, it is useful to provide the app elapse time.

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@turboFei turboFei changed the title Open time Return app submission time for batch Jan 7, 2023
@turboFei
Copy link
Member Author

turboFei commented Jan 7, 2023 •
edited
Loading

also cc @lightning-L
This pr has metadata schema change, it depends on #4022

@turboFei turboFei self-assigned this Jan 7, 2023
@turboFei turboFei added this to the v1.7.0 milestone Jan 7, 2023
@turboFei turboFei closed this in 1499525 Jan 8, 2023
@turboFei
Copy link
Member Author

turboFei commented Jan 8, 2023

thanks, merged to master

@turboFei turboFei deleted the open_time branch January 8, 2023 09:47
ulysses-you
ulysses-you reviewed Jan 9, 2023
View reviewed changes
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala
applicationManager.getApplicationInfo(builder.clusterManager(), batchId).filter(_.id != null)
applicationInfo.foreach { _ =>
if (_appSubmissionTime <= 0) {
_appSubmissionTime = System.currentTimeMillis()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it's the appStartTime ? Submisstion time is the time before submiting succeed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we donot set it in submitAndMonitorBatchJob method ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we donot set it in submitAndMonitorBatchJob method ?

Because in the submitAndMonitorBatchJob method, there is interval to track the app state.
And for the getBatch api, it will also call currentApplicationState to build the batch report, to keep the state align, it is better to update the app submission time in currentApplicationState.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make state machine and the time trace consistent.

If the submitssion time means when the state changes to running, then we should update the time around this code

setStateIfNotCanceled(OperationState.RUNNING)

Otherwise the client may get the batch info with submisstion time = 0 but state is running or submisstion time != 0 but state is pending.

Copy link
Member Author

@turboFei turboFei Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for batch report, it is consistent with the appId/appUrl/appState now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can treat it as a filed of batch application info.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is the pr for followup #4129

@turboFei turboFei mentioned this pull request Jan 9, 2023
Closed
3 tasks
turboFei added a commit that referenced this pull request Jan 9, 2023
@turboFei
[KYUUBI #4119][FOLLOWUP] Refactor appSubmissionTime to appStartTime
cd3916d 
### _Why are the changes needed?_

To address comment #4119 (comment)

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4129 from turboFei/4119_followup.

Closes #4119

30626e5 [fwang12] appSubmissionTime to appStartTime

Authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: fwang12 <fwang12@ebay.com>
turboFei added a commit that referenced this pull request Jan 29, 2023
@turboFei
[KYUUBI #4119][FOLLOWUP] Do not fail on unknown properties to keep ky…
750eb6a 
…uubi-ctl compatiblity

### _Why are the changes needed?_

```
Caused by: com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "appStartTime" (class org.apache.kyuubi.client.api.v1.dto.Batch), not marked as ignorable (13 known properties: "appDiagnostic", "createTime", "kyuubiInstance", "user", "appUrl", "appState", "state", "batchType", "cluster", "id", "appId", "endTime", "name"])
```

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4208 from turboFei/ignore_unknown.

Closes #4119

0e81ac7 [fwang12] save

Authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: fwang12 <fwang12@ebay.com>
turboFei added a commit that referenced this pull request Jan 29, 2023
@turboFei
[KYUUBI #4119][FOLLOWUP] Do not fail on unknown properties to keep ky…
cec0694 
…uubi-ctl compatiblity

### _Why are the changes needed?_

```
Caused by: com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "appStartTime" (class org.apache.kyuubi.client.api.v1.dto.Batch), not marked as ignorable (13 known properties: "appDiagnostic", "createTime", "kyuubiInstance", "user", "appUrl", "appState", "state", "batchType", "cluster", "id", "appId", "endTime", "name"])
```

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4208 from turboFei/ignore_unknown.

Closes #4119

0e81ac7 [fwang12] save

Authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: fwang12 <fwang12@ebay.com>
(cherry picked from commit 750eb6a)
Signed-off-by: fwang12 <fwang12@ebay.com>
@turboFei turboFei mentioned this pull request Feb 18, 2023
Closed
3 tasks
pan3793 pushed a commit that referenced this pull request Feb 18, 2023
@turboFei @pan3793
[KYUUBI #4119][FOLLOWUP] Add app start time for batch api docs
e647cc9 
### _Why are the changes needed?_

#4119 follow up

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [ ] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4365 from turboFei/app_start.

Closes #4119

3cf7e7a [fwang12] add app start time

Authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
pan3793 pushed a commit that referenced this pull request Feb 18, 2023
@turboFei @pan3793
[KYUUBI #4119][FOLLOWUP] Add app start time for batch api docs
35824f5 
### _Why are the changes needed?_

#4119 follow up

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [ ] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4365 from turboFei/app_start.

Closes #4119

3cf7e7a [fwang12] add app start time

Authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit e647cc9)
Signed-off-by: Cheng Pan <chengpan@apache.org>
SusurHe pushed a commit to SusurHe/incubator-kyuubi that referenced this pull request Feb 21, 2023
Merge branch 'master' of github.com:apache/kyuubi
4cd3d23 
* 'master' of github.com:apache/kyuubi: (1557 commits)
  [KYUUBI apache#3951][FOLLOWUP] Audit the rest request params
  [KYUUBI apache#4377] Grant execute permission to release scripts
  [KYUUBI apache#4374] Release uploading should include kyuubi-spark-connector-hive
  [KYUUBI apache#4267] Show warning if SessionHandle is invalid
  [KYUUBI apache#4385] [DOCS] Refine release process
  [KYUUBI apache#4352] Support System.gc() with periodic GC interval
  [KYUUBI apache#4152][FOLLOWUP] LDAP configurations should be server-only
  [KYUUBI apache#4373] Using SVN_STAGING_REPO instead of SVN_STAGING_REPO in the release script to fix echo message
  [KYUUBI apache#4372] Support to return null value for OperationsResource rowset
  [KYUUBI apache#4371] Fix typo in `kyuubi_ecosystem.drawio`
  [KYUUBI apache#4216] Support to transfer client version for kyuubi hive jdbc and rest client sdk
  [KYUUBI apache#4345] Add the doc of kyuubi trino server
  [KYUUBI apache#3081][DOCS] Add Hudi connector doc in Trino
  [KYUUBI apache#4357] Bump Jersey from 2.38 to 2.39
  [KYUUBI apache#4338][FOLLOWUP] Fix K8s integration tests
  [KYUUBI apache#4326] [ARROW] Fix Spark session timezone format in arrow-based result format
  [KYUUBI apache#4360][FOLLOWUP] Get valid unlimited users from existing limiters instead of conf
  [KYUUBI apache#4362] Add `_configurations` in kerberos.rst
  [KYUUBI apache#4338] Bump Spark from 3.3.1 to 3.3.2
  [KYUUBI apache#4119][FOLLOWUP] Add app start time for batch api docs
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ulysses-you ulysses-you ulysses-you left review comments

@pan3793 pan3793 pan3793 approved these changes

Assignees

@turboFei turboFei

Labels
module:ctl module:rest-client module:server
Projects
None yet
Milestone
v1.7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@turboFei @ulysses-you @pan3793