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

Mention the path of DLQ to indicate DLQ if full for which pipeline #11280

Closed

Conversation

amitavmohanty01
Copy link
Contributor

A series of messages of the following form do not indicate DLQ for which pipeline is full.
[2019-10-30T11:53:33,801][ERROR][org.logstash.common.io.DeadLetterQueueWriter] cannot write event to DLQ: reached maxQueueSize of 1073741824

So, it is probably better to let the user know the path of the DLQ file so that the user can:

  1. know which pipeline is affected
  2. clear the DLQ files manually if appropriate

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@amitavmohanty01
Copy link
Contributor Author

@danhermann please provide feedback on the approach and implementation

@@ -131,7 +131,7 @@ private void innerWriteEntry(DLQEntry entry) throws IOException {
byte[] record = entry.serialize();
int eventPayloadSize = RECORD_HEADER_SIZE + record.length;
if (currentQueueSize.longValue() + eventPayloadSize > maxQueueSize) {
logger.error("cannot write event to DLQ: reached maxQueueSize of " + maxQueueSize);
logger.error("cannot write event to DLQ(path:"+ this.queuePath +"): reached maxQueueSize of " + maxQueueSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

@amitavmohanty01, this looks pretty good to me. There are a couple code formatting issues, but those are pretty minor. The line above should include spaces in the places where I inserted underscores below:

logger.error("cannot write event to DLQ_(path:_"_+ this.queuePath +_"): reached maxQueueSize of " + maxQueueSize);

I'll need to defer to someone on the @elastic/logstash team to approve and merge this PR, though.

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 have modified as suggested and edited the commit.

@andsel
Copy link
Contributor

andsel commented Oct 31, 2019

@amitavmohanty01 with PR #11075 there should already be the name of failing pipeline in the logs which cover point 1).
This PR is good for the point 2) where the user could now where is the directory to cleanup

@amitavmohanty01
Copy link
Contributor Author

@andsel thanks for the inputs. I am in agreement.

Can you please trigger a Jenkins build so that we can be sure there is nothing missed out?

@andsel
Copy link
Contributor

andsel commented Oct 31, 2019

Jenkins test this please

@andsel
Copy link
Contributor

andsel commented Oct 31, 2019

@amitavmohanty01 The error should be resolved if you align the code to master (latest commits solved it):

$ git checkout master
$ git fetch upstream
$ git merge upstream/master
$ git checkout dlq_file_path
$ git rebase master
$ git push origin dlq_file_path -f

@amitavmohanty01
Copy link
Contributor Author

@andsel I have rebased on master. Please trigger the Jenkins build again.

@amitavmohanty01
Copy link
Contributor Author

@andsel ping

@andsel
Copy link
Contributor

andsel commented Nov 6, 2019

Jenkins test this please

@amitavmohanty01
Copy link
Contributor Author

@andsel it seems the errors on Jenkins are unrelated to my code. Am I missing something?

@andsel
Copy link
Contributor

andsel commented Nov 7, 2019

@amitavmohanty01 to me seems all ok, I'll asking Jenkins to rebuild it. If it fails again probably there is a subtle dependency between the two facts to spoil

@andsel
Copy link
Contributor

andsel commented Nov 7, 2019

Jenkins test this please

@amitavmohanty01
Copy link
Contributor Author

amitavmohanty01 commented Nov 10, 2019

@andsel I ran ./gradlew test locally and the result was the following.

----------------------------------------------------------------------
|  Results: SUCCESS (56 tests, 56 successes, 0 failures, 0 skipped)  |
----------------------------------------------------------------------

Is the Jenkins environment having any nuances causing the failures? I am considering that especially because the failure and the code change seem unrelated.

@andsel
Copy link
Contributor

andsel commented Nov 11, 2019

@amitavmohanty01 to build LogStash you have to execute ./gradlew build from the checkout directory. But the Gradle task that fails is :logstash-xpack:rubyIntegrationTests. It's not related to your PR but something to Elasticsearch master (elastic/elasticsearch#48170)

@andsel andsel self-requested a review November 11, 2019 10:28
@andsel
Copy link
Contributor

andsel commented Nov 12, 2019

@amitavmohanty01 with the integration of PR #11297 the issue is fixed. Please could you rebase your branch to the current master?

@amitavmohanty01
Copy link
Contributor Author

@andsel done

@andsel
Copy link
Contributor

andsel commented Nov 12, 2019

Jenkins test this please

2 similar comments
@andsel
Copy link
Contributor

andsel commented Nov 13, 2019

Jenkins test this please

@andsel
Copy link
Contributor

andsel commented Nov 13, 2019

Jenkins test this please

@elasticsearch-bot
Copy link

Andrea Selva merged this into the following branches!

Branch Commits
master b99c2f9
7.x 3dd117a

@colinsurprenant
Copy link
Contributor

Adding a note here to not forget to backport on 7.5.0 once released so that it gets into 7.5.1 since we are late in the 7.5.0 release process.

@colinsurprenant
Copy link
Contributor

@andsel also, we should backport to 7.4 branch!

@elasticsearch-bot
Copy link

Andrea Selva merged this into the following branches!

Branch Commits
7.4 d65ef8e

@elasticsearch-bot
Copy link

João Duarte merged this into the following branches!

Branch Commits
7.5 b50ff4f

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

Successfully merging this pull request may close these issues.

7 participants