Skip to content

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Aug 18, 2020

What changes were proposed in this pull request?

This PR proposes to upload target/unit-tests.log into the artifact so it will be able to download here:
Screen Shot 2020-08-18 at 2 23 18 PM

Why are the changes needed?

Jenkins has this feature. It should be best to have the same dev functionalities with it.
Also, note that this was pointed out #29225 (comment).

Does this PR introduce any user-facing change?

No, dev-only

How was this patch tested?

https://github.com/apache/spark/actions/runs/213000777 should demonstrate it

name: test-results-${{ matrix.modules }}-${{ matrix.comment }}-${{ matrix.java }}-${{ matrix.hadoop }}-${{ matrix.hive }}
path: "**/target/test-reports/*.xml"
- name: Upload unit tests log files
if: failure()
Copy link
Member Author

@HyukjinKwon HyukjinKwon Aug 18, 2020

Choose a reason for hiding this comment

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

Zipped logfiles are ~300 MB if we collect all. Jenkins always uploads but GitHub Actions uploads failed tests only for now.

I can't find the clear answer of the size limit from my googling but seems like there's a limit internally. Should be best to save the resources.

Copy link
Member

Choose a reason for hiding this comment

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

Logs for failed tests only should be good enough from my experience.

Copy link
Member

Choose a reason for hiding this comment

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

With if: failure(), only the failed test logs are supposed to be uploaded, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup yup.

@SparkQA

This comment has been minimized.

@HyukjinKwon
Copy link
Member Author

cc @Ngone51 and @tgravescs FYI. This is being tested at https://github.com/apache/spark/actions/runs/213000777

Copy link
Member

@Ngone51 Ngone51 left a comment

Choose a reason for hiding this comment

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

Thank you @HyukjinKwon for add this!!

name: test-results-${{ matrix.modules }}-${{ matrix.comment }}-${{ matrix.java }}-${{ matrix.hadoop }}-${{ matrix.hive }}
path: "**/target/test-reports/*.xml"
- name: Upload unit tests log files
if: failure()
Copy link
Member

Choose a reason for hiding this comment

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

Logs for failed tests only should be good enough from my experience.

@HyukjinKwon
Copy link
Member Author

Should be ready for a look or possibly to be merged.

@SparkQA
Copy link

SparkQA commented Aug 18, 2020

Test build #127523 has finished for PR 29454 at commit bd7ff11.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

cc @dongjoon-hyun, @gengliangwang, @viirya FYI

@Ngone51
Copy link
Member

Ngone51 commented Aug 18, 2020

I've downloaded the log from: https://github.com/apache/spark/actions/runs/213000777 and it looks good!

LGTM

@SparkQA
Copy link

SparkQA commented Aug 18, 2020

Test build #127565 has finished for PR 29454 at commit d787cd1.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Looks good. Do we know how long the artifacts will be kept?

@HyukjinKwon
Copy link
Member Author

GitHub keeps 90 days (https://docs.github.com/en/actions/configuring-and-managing-workflows/managing-a-workflow-run#viewing-logs-to-diagnose-failures) but I think there's another internal limit .. given my random googling.

if: failure()
uses: actions/upload-artifact@v2
with:
name: unit-tests-log-${{ matrix.modules }}-${{ matrix.comment }}-${{ matrix.java }}-${{ matrix.hadoop }}-${{ matrix.hive }}
Copy link
Member

Choose a reason for hiding this comment

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

nit though, adding a prefix jdk for matrix.java will make the log name clearer just like hadoop3.2 and hive2.3?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. I plan to have a proper name instead of combination of the matrix to address such issue clearly. I'll make a separate PR to address the issue of all such names in this whole file later.

@maropu
Copy link
Member

maropu commented Aug 19, 2020

Nice feature!

@HyukjinKwon
Copy link
Member Author

Thanks guys. Merged to master.

HyukjinKwon added a commit to HyukjinKwon/spark that referenced this pull request Aug 19, 2020
### What changes were proposed in this pull request?

This PR proposes to upload `target/unit-tests.log` into the artifact so it will be able to download here:
![Screen Shot 2020-08-18 at 2 23 18 PM](https://user-images.githubusercontent.com/6477701/90474095-789e3b80-e15f-11ea-87f8-e7da3df3c03e.png)

### Why are the changes needed?

Jenkins has this feature. It should be best to have the same dev functionalities with it.
Also, note that this was pointed out apache#29225 (comment).

### Does this PR introduce _any_ user-facing change?

No, dev-only

### How was this patch tested?

https://github.com/apache/spark/actions/runs/213000777 should demonstrate it

Closes apache#29454 from HyukjinKwon/SPARK-32645.

Authored-by: HyukjinKwon <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
HyukjinKwon added a commit to HyukjinKwon/spark that referenced this pull request Aug 19, 2020
### What changes were proposed in this pull request?

This PR proposes to upload `target/unit-tests.log` into the artifact so it will be able to download here:
![Screen Shot 2020-08-18 at 2 23 18 PM](https://user-images.githubusercontent.com/6477701/90474095-789e3b80-e15f-11ea-87f8-e7da3df3c03e.png)

### Why are the changes needed?

Jenkins has this feature. It should be best to have the same dev functionalities with it.
Also, note that this was pointed out apache#29225 (comment).

### Does this PR introduce _any_ user-facing change?

No, dev-only

### How was this patch tested?

https://github.com/apache/spark/actions/runs/213000777 should demonstrate it

Closes apache#29454 from HyukjinKwon/SPARK-32645.

Authored-by: HyukjinKwon <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
HyukjinKwon added a commit to HyukjinKwon/spark that referenced this pull request Aug 19, 2020
### What changes were proposed in this pull request?

This PR proposes to upload `target/unit-tests.log` into the artifact so it will be able to download here:
![Screen Shot 2020-08-18 at 2 23 18 PM](https://user-images.githubusercontent.com/6477701/90474095-789e3b80-e15f-11ea-87f8-e7da3df3c03e.png)

### Why are the changes needed?

Jenkins has this feature. It should be best to have the same dev functionalities with it.
Also, note that this was pointed out apache#29225 (comment).

### Does this PR introduce _any_ user-facing change?

No, dev-only

### How was this patch tested?

https://github.com/apache/spark/actions/runs/213000777 should demonstrate it

Closes apache#29454 from HyukjinKwon/SPARK-32645.

Authored-by: HyukjinKwon <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
@HyukjinKwon HyukjinKwon deleted the SPARK-32645 branch December 7, 2020 02:06
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.

6 participants