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

DRILL-6478: enhance debug logs for batch sizing #1310

Closed
wants to merge 1 commit into from

Conversation

@ppadma
Copy link
Contributor

commented Jun 7, 2018

@kkhatua simple change. can you quickly review ?

@kkhatua

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

@ppadma looks like the primary enhancement is to have the logging limited to a single line. If that is it, then it LGTM. +1

@sohami

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

@ppadma - Can you please look into the travis failures ?

@ppadma ppadma force-pushed the ppadma:DRILL-6478 branch from 38a68de to f9fde24 Jun 8, 2018
@ppadma

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2018

@sohami it is some timeout issue. I built and ran tests fine on my local mac.

@@ -171,6 +171,10 @@ public FlattenRecordBatch(FlattenPOP pop, RecordBatch incoming, FragmentContext
// get the output batch size from config.
int configuredBatchSize = (int) context.getOptions().getOption(ExecConstants.OUTPUT_BATCH_SIZE_VALIDATOR);
flattenMemoryManager = new FlattenMemoryManager(configuredBatchSize);

if (logger.isDebugEnabled()) {

This comment has been minimized.

Copy link
@sohami

sohami Jun 8, 2018

Contributor

Same as above for if condition

@@ -890,6 +890,9 @@ public HashJoinBatch(HashJoinPOP popConfig, FragmentContext context,
// get the output batch size from config.
int configuredBatchSize = (int) context.getOptions().getOption(ExecConstants.OUTPUT_BATCH_SIZE_VALIDATOR);
batchMemoryManager = new JoinBatchMemoryManager(configuredBatchSize, left, right);
if (logger.isDebugEnabled()) {

This comment has been minimized.

Copy link
@sohami

sohami Jun 8, 2018

Contributor

check not needed here.

final int configuredBatchSize = (int) context.getOptions().getOption(ExecConstants.OUTPUT_BATCH_SIZE_VALIDATOR);
batchMemoryManager = new MergeJoinMemoryManager(configuredBatchSize, left, right);

if (logger.isDebugEnabled()) {

This comment has been minimized.

Copy link
@sohami

sohami Jun 8, 2018

Contributor

if condition not needed.

@@ -74,6 +76,9 @@ public UnionAllRecordBatch(UnionAll config, List<RecordBatch> children, Fragment
// get the output batch size from config.
int configuredBatchSize = (int) context.getOptions().getOption(ExecConstants.OUTPUT_BATCH_SIZE_VALIDATOR);
batchMemoryManager = new RecordBatchMemoryManager(numInputs, configuredBatchSize);
if (logger.isDebugEnabled()) {

This comment has been minimized.

Copy link
@sohami

sohami Jun 8, 2018

Contributor

Same as above

@@ -409,6 +426,22 @@ public void remove() {
public void close() {
super.close();
updateBatchMemoryManagerStats();

logger.debug("BATCH_STATS, incoming aggregate left: batch count : {}, avg bytes : {}, avg row bytes : {}, record count : {}",

This comment has been minimized.

Copy link
@sohami

sohami Jun 8, 2018

Contributor

this entire block you can put under the check if(logger.isDebugEnabled())

This comment has been minimized.

Copy link
@ppadma

ppadma Jun 8, 2018

Author Contributor

This does not have to be under isDebugEnabled flag

This comment has been minimized.

Copy link
@sohami

sohami Jun 11, 2018

Contributor

The reason for putting it is so that for each log statement internally it doesn't check for debug level being enabled. Having single outer check will help here. Actually same is the case with other operators close method too. Would be good to update those as well

@@ -158,7 +158,7 @@ public void update() {
setOutputRowCount(Math.min(columnSize.getElementCount(), getOutputRowCount()));

if (logger.isDebugEnabled()) {

This comment has been minimized.

Copy link
@sohami

sohami Jun 8, 2018

Contributor

if condition is not needed.

@@ -123,7 +123,7 @@
public void update(int inputIndex) {
status.setTargetOutputRowCount(super.update(inputIndex, status.getOutPosition()));
if (logger.isDebugEnabled()) {

This comment has been minimized.

Copy link
@sohami

sohami Jun 8, 2018

Contributor

if condition not needed.

@@ -361,6 +370,10 @@ public boolean hasNext() {
if (topStatus.prefetched) {
topStatus.prefetched = false;
batchMemoryManager.update(topStatus.batch, topStatus.inputIndex);
if (logger.isDebugEnabled()) {

This comment has been minimized.

Copy link
@sohami

sohami Jun 8, 2018

Contributor

if condition not needed.

@@ -378,6 +391,10 @@ public boolean hasNext() {
topStatus.recordsProcessed = 0;
topStatus.totalRecordsToProcess = topStatus.batch.getRecordCount();
batchMemoryManager.update(topStatus.batch, topStatus.inputIndex);
if (logger.isDebugEnabled()) {

This comment has been minimized.

Copy link
@sohami

sohami Jun 8, 2018

Contributor

please remove if condition here too.

@arina-ielchiieva

This comment has been minimized.

Copy link
Member

commented Jun 8, 2018

@ppadma you always can re-start Travis in case build failed with timeout or any other unrelated failure.
image

@ilooner

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2018

@arina-ielchiieva That button is only enabled for committers. If your not a committer the only way to restart the build is to close and open the PR or to push a new commit.

@ppadma ppadma force-pushed the ppadma:DRILL-6478 branch from f9fde24 to 07ae755 Jun 8, 2018
@ppadma

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2018

@sohami addressed review comments. please take a look.

@ppadma ppadma force-pushed the ppadma:DRILL-6478 branch 3 times, most recently from ca6e323 to 31035b0 Jun 11, 2018
@ppadma

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

@sohami ok, made the change.

@ppadma ppadma force-pushed the ppadma:DRILL-6478 branch from 31035b0 to 2b39afd Jun 11, 2018
@sohami

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

LGTM +1

@ilooner ilooner closed this in cbcb59d Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.