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

Excel streaming xlsx option with new configuration UserProperty "ExcelEmitter.StreamingXlsx" #1322 #1323

Merged

Conversation

speckyspooky
Copy link
Contributor

The new user property give the option to define the kind of workbook processing which should be used.
The new option: ExcelEmitter.StreamingXlsx is a boolean and has ethe options true, false (default: false).
The values are:

  • true: activate the SXSSF usage to stream the data internally
  • false: standard handling based on dom object

The difference of memory usage is given but the handling isn't the same in all steps.
My test with 1Mio records:
XSSF: 130s
SXSSF: 82s

Attached my test case with 1Mio records:

Data7602DescendingYearOrder-shrinked-1Mio.zip

@speckyspooky speckyspooky added this to the 4.14 milestone Jun 9, 2023
@speckyspooky speckyspooky added the Enhancement Small change to improve the current supported functionality label Jun 9, 2023
@speckyspooky speckyspooky self-assigned this Jun 9, 2023
@speckyspooky
Copy link
Contributor Author

@wimjongman
Sorry, this PR includes the changes of #1307 again the changes includes only 3 files.
I don't know why this happens because I created a new branch and eclupse showed only the 3 changed files:

2023-06-10 00_18_43-Excel streaming xlsx option with new configuration UserProperty _ExcelEmitter St

@merks
Copy link
Contributor

merks commented Jun 10, 2023

@speckyspooky

When you created the new branch you created it when the checked out branch at the time of creation had commits on it other than those on the remote master of the main repository. I don't think you can commit the changes here without including both the commits...

Generally, if you want to keep things separate, you should check out master, and pull before you create a new branch.

You could "fix" this by renaming the branch used for this PR, checking out master, pulling, creating a new branch with this same name as you used before, in the history view, cherry pick this commit onto that branch, and finally force push that branch to your fork which will update this PR...

@speckyspooky
Copy link
Contributor Author

Yes, your expanding make sense.
I created the new branch directly.

The good thing user both changes should be part of 4.14. and there are no further changes.

@merks
Copy link
Contributor

merks commented Jun 10, 2023

@speckyspooky

In my opinion the entire set of changes looks fine and good! I think they should be merged, and that might be a good idea to do sooner rather than later because I also have a commit pending which I don't believe will conflict, but you never know...

If @wimjongman has suggestions, they can always be applied after the fact...

@speckyspooky
Copy link
Contributor Author

I will merge the changes this weekend.
The risk is very small and I want to go ahead with viewer changes.

@wimjongman
Copy link
Contributor

wimjongman commented Jun 10, 2023 via email

@speckyspooky speckyspooky merged commit 484ab59 into eclipse-birt:master Jun 10, 2023
2 checks passed
@speckyspooky
Copy link
Contributor Author

@merks
It seems to me that I have an issue with the new build process.
The build seems to be done but there is an error documented on Jenkins. Could it be that I need additona grants/privileges?

Could you please assist what I can do to fix it or get the additional grants.

2023-06-10 15_07_51-Eclipse Business Intelligence and Reporting Tools (BIRT) - build Scan Repository

@merks
Copy link
Contributor

merks commented Jun 10, 2023

There seemed to be a glitch (infrastructure problem) in a PR build as well and I just restarted the build manually and now that seems to be doing it's thing:

https://ci.eclipse.org/birt/job/build/job/PR-1325/2/console

I kicked off master again manually and it seems to be waiting for an executor:

https://ci.eclipse.org/birt/job/build/job/master/12/console

Which makes sense because there are two executors running already and that's the max:

image

@wimjongman

We could disable the old builds I've underlined here and eventually delete them:

image

The birt-master is especially annoying because it kicks in automatically at the same time as "build" and by using an executor blocks other jobs, e.g., PR builds. I.e., that is superseded by which does the publishing to the new locations:

https://ci.eclipse.org/birt/job/build/job/master/

@merks
Copy link
Contributor

merks commented Jun 10, 2023

I've disabled all those builds and I've abort the birt-master build. The https://ci.eclipse.org/birt/job/birt-master/306/console job says it's aborted:

Build was aborted
Aborted by [Ed Merks](https://ci.eclipse.org/birt/user/ed.merks@gmail.com)
Skipped archiving because build is not successful
Recording test results
Finished: ABORTED

But it's not cleared off of the main page yet:

image

Where the build -> master is still showing in the queue.

Unfortunately the ci instance is not always as well behaved as it ought to be. I'll check the status again in while...

@merks
Copy link
Contributor

merks commented Jun 10, 2023

I merged my PR because the GitHub action completed and verified it, so that job is killed and now master is building:

https://ci.eclipse.org/birt/job/build/job/master/13/console

which will include both the commit for this PR and the commit for my PR...

@speckyspooky
Copy link
Contributor Author

Ok, I have seen the cancelled jobs from your side and also your new job of your PR.
The original build of my PR was successful but due to the Jenkins-jobs which caused the current situation it wasn't finished at all.

So I cross my fingures that your new jobs works well :o)

@merks
Copy link
Contributor

merks commented Jun 10, 2023

It finished and published a new build:

https://download.eclipse.org/birt/updates/nightly/latest

@speckyspooky
Copy link
Contributor Author

Many thanks to your side!

@speckyspooky speckyspooky deleted the excel_streaming_xlsx_#1322 branch September 16, 2023 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Small change to improve the current supported functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants