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

Bugfix/233 store as binary #234

Merged
merged 4 commits into from
May 29, 2024

Conversation

pcastelog
Copy link

Will solve #233

Copy link
Collaborator

@nhirrle nhirrle left a comment

Choose a reason for hiding this comment

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

thanks a lot for the fix. I was about to do the same but got disrupted than.
left 2 minor comments

@@ -77,6 +86,9 @@ public class HistoryUtil {
protected static final String ATTR_START = "start";
protected static final String ATTR_END = "end";
private static final String NAME_INDEX = "oak:index";
// This size is limited by the LuceneDocumentMaker to be able to read the property and create the new index
// The limit is 102400 but just to be in the safe side, is set to a bit lower number
private static final int MAXIMUN_PROPERTY_SIZE = 100000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: MAXIMUN_PROPERTY_SIZE -> MAXIMUM_PROPERTY_SIZE

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@@ -358,7 +370,21 @@ private void saveExecutionResultInHistory(ExecutionResult result, String path, R
values.put(ATTR_RUN_STATE, result.getState().name());
values.put(ATTR_PATH, result.getPath());
if (StringUtils.isNotBlank(result.getOutput())) {
values.put(ATTR_RUN_OUTPUT, result.getOutput());
if (result.getOutput().getBytes(StandardCharsets.UTF_8).length < MAXIMUN_PROPERTY_SIZE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

small change suggestion: store full data in seperate property in case it is too big. like that we don't need to check type of the property when reading.

if (result.getOutput().getBytes(StandardCharsets.UTF_8).length > MAXIMUM_PROPERTY_SIZE) {
    values.put(ATTR_RUN_OUTPUT, "Output data too big, full data is stored as a binary in runOutputFull");
    values.put(ATTR_RUN_OUTPUT_FULL, new ByteArrayInputStream(result.getOutput().getBytes(StandardCharsets.UTF_8)));
    LOG.info("Script result is bigger than 100 000 bytes. Full data can be found as a binary in property runOutputFull for path " + path );
}

Copy link
Author

Choose a reason for hiding this comment

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

Added the new property

@nhirrle nhirrle merged commit f09c0f9 into valtech:develop May 29, 2024
1 check passed
@pcastelog
Copy link
Author

I would appreciate if you could publish a minor version with the latest changes
This one and the fix for the cloud readiness improvement

Thanks!

@nhirrle
Copy link
Collaborator

nhirrle commented May 30, 2024

just released it, thanks for your contributions @pcastelog !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants