Skip to content

Conversation

@haozhun
Copy link
Contributor

@haozhun haozhun commented Oct 6, 2017

No description provided.

Copy link
Contributor

@electrum electrum left a comment

Choose a reason for hiding this comment

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

See Travis failures

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use a stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The next commit introduces side effect to the for-loop

Copy link
Contributor

Choose a reason for hiding this comment

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

Why times two? There is only one String in the class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing @Override

Copy link
Contributor

Choose a reason for hiding this comment

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

Just add

.map(Object::toString)

Copy link
Contributor

Choose a reason for hiding this comment

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

.map(Object::toString)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is long and hard to read, add parentheses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree on this one. I find it harder to read with parenthesis.

I will address all other comments. I noticed the build failure and fixed them myself. Apparently, I forgot to push it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use toIntExact

Copy link
Contributor

Choose a reason for hiding this comment

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

Using Ints.saturatedCast() might be better here. It means the memory limit will be lower than the very large limit that was specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can overflow if the limit is close to MAX_INT, and/or if there are many threads incrementing at once

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably make this a long, just to be safe

Copy link
Contributor

Choose a reason for hiding this comment

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

format(
"Split buffering for %s.%s exceeded memory limit (%s). %s splits are buffered.",
..., succinctBytes(maxOutstandingSplitsBytes), ...)

@haozhun haozhun force-pushed the hive-split branch 4 times, most recently from 924793b to 51fc023 Compare October 12, 2017 02:04
InternalHiveSplit avoids unnecessary duplicate information, and is
more friendly to memory accounting. It is used for buffering discovered
splits inside Hive connector.
@haozhun haozhun merged commit 1e49d9b into prestodb:master Oct 16, 2017
@wenleix
Copy link
Contributor

wenleix commented Apr 6, 2019

Related PR: #9175, #9232

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.

4 participants