Skip to content

Conversation

@chaitanyaphalak
Copy link
Contributor

No description provided.

@dtregonning dtregonning self-requested a review August 16, 2018 23:42

if (channelEvents.get(resp.getAckId()) != null) {
log.error("ackId={} already exists for channel={} index={}", resp.getAckId(), channel, channel.getIndexer());
log.warn("ackId={} already exists for channel={} index={}, possible duplication of data", resp.getAckId(), channel, channel.getIndexer());
Copy link
Contributor

Choose a reason for hiding this comment

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

Modify to
log.warn("ackId={} already exists for channel={} index={} data may be duplicated in Splunk", resp.getAckId(), channel, channel.getIndexer());

return ackPollInterval;
}

public void stickySessionHandler(HecChannel channel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Include JavaDocs for new method

/**
 * StickySessionHandler is .... It is used to .. 
 *
 * @param  channel  "channel is ...
 * @see          HecChannel
 * @since        1.1.0
 */

ConcurrentHashMap<Long, EventBatch> channelBatches = outstandingEventBatches.get(channel);
// Remove batches for the channel from the poller
if(channelBatches != null && channelBatches.size() > 0) {
totalOutstandingEventBatches.addAndGet(-channelBatches.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly use getAndDecrement() after each batch.fail instead of larger subtraction of totalOutstandingEventBatches to ensure correctness in value at all times.

batch.fail();
expired.add(batch);
iter.remove();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

include log lines indicated - Found x batches marked for failure
- Sucessfully failed x batches

if(resp.equals("sticky_session_expired")) {
stickySessionHandler(channel);
} else {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need to nest the try catch. move it out to capture
String resp = channel.executeHttpRequest(request);
and
stickySessionHandler(channel);


public void setId() { id = newChannelId(); }

public void setAvailable(boolean _isAvailable) { isAvailable = _isAvailable; }
Copy link
Contributor

Choose a reason for hiding this comment

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

change to

public void setAvailable(boolean isAvailable) { this.isAvailable = isAvailable; }

log.debug("sent {} events to splunk through channel={} indexer={}",
batch.size(), channel.getId(), getBaseUrl());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra line

Copy link
Contributor

@dtregonning dtregonning left a comment

Choose a reason for hiding this comment

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

Thanks for PR Chaitanya.

Please address comments in review and validate duplicate testing for current release and PR are equal or better.

@chaitanyaphalak chaitanyaphalak force-pushed the channel-expiration-logging branch 4 times, most recently from cbf4c56 to 17adf42 Compare August 20, 2018 22:16
} else if (resp.equals(noDataError)) {
return createResponse(resp, 400);
} else {
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove space

}

// log.info("event posting, channel={}, cookies={}", channel, resp.getHeaders("Set-Cookie"));
// log.info("event posting, channel={}, cookies={}, cookies.length={}", channel, resp.getHeaders("Set-Cookie"), resp.getHeaders("Set-Cookie").length);
Copy link
Contributor

Choose a reason for hiding this comment

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

realign // in comment

// Set channel unavailable
channel.setAvailable(false);
log.info("Channel {} set to be not available", oldChannelId);
// Get batches for the channel
Copy link
Contributor

Choose a reason for hiding this comment

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

move single line comments to "flow description" in java docs

return;
}
handleAckPollResult(channel, ackPollResult);
if (ackPollResult != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

look for more graceful way of handling null response. use exception handling or remove if not needed

@chaitanyaphalak chaitanyaphalak force-pushed the channel-expiration-logging branch from 17adf42 to 33306e8 Compare August 21, 2018 16:47
Copy link
Contributor

@dtregonning dtregonning left a comment

Choose a reason for hiding this comment

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

Thanks for PR - all work looks good.

@dtregonning dtregonning merged commit d789ce1 into develop Aug 21, 2018
@dtregonning dtregonning deleted the channel-expiration-logging branch August 21, 2018 20:29
@chaitanyaphalak chaitanyaphalak restored the channel-expiration-logging branch August 21, 2018 21:43
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.

2 participants