You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/04/19 07:53:17 UTC

[GitHub] [skywalking] yangyiweigege opened a new pull request, #8909: fix ES flush thread will stop work when flush schedule task have exception(#8852)

yangyiweigege opened a new pull request, #8909:
URL: https://github.com/apache/skywalking/pull/8909

   …ption(#8895)
   
   <!--
       ⚠️ Please make sure to read this template first, pull requests that don't accord with this template
       maybe closed without notice.
       Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
       Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
   -->
   
   <!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
   ### Fix <bug description or the bug issue number or bug issue link>
   - [ ] Add a unit test to verify that the fix works.
   - [ ] Explain briefly why the bug exists and how to fix it.
        ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
   
   <!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
   ### Improve the performance of <class or module or ...>
   - [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
   - [ ] The benchmark result.
   ```text
   <Paste the benchmark results here>
   ```
   - [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
        ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
   
   <!-- ==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👇 ====
   ### <Feature description>
   - [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
   - [ ] Update the documentation to include this new feature.
   - [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
   - [ ] If it's UI related, attach the screenshots below.
        ==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👆 ==== -->
   
   - [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
   - [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/changelog/docs/en/changes/changes.md).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] yangyiweigege commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

yangyiweigege commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102281638

   > I think you should be clear about which exception could cause this? And you should follow the PR template, linking to issue and update change log.
   
   i have changed the title  and linking to linking to issue and update change log.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102296651

    **I think you should be clear about which exception could cause this? ** This is a key question. @kezhenxu94 showed concerns in the issue, which should be answered before any code change.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on code in PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#discussion_r852917303


##########
docs/en/changes/changes.md:
##########
@@ -20,6 +20,7 @@
 * Set `SW_QUERY_MAX_QUERY_COMPLEXITY` default value to `1000`
 * Webapp module (for UI) enabled compression.
 * [Breaking Change] Add layer field to event, report an event without layer is not allowed.
+* Fix ES flush thread will stop when flush schedule task have exception.

Review Comment:
   ```suggestion
   * Fix ES flush thread stops when flush schedule task throws exception, such as ElasticSearch flush failed.
   ```
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng merged pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

wu-sheng merged PR #8909:
URL: https://github.com/apache/skywalking/pull/8909


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] yangyiweigege commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

yangyiweigege commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102365895

   > **I think you should be clear about which exception could cause this?** This is a key question. @kezhenxu94 showed concerns in the issue, which should be answered before any code change.
   
   
   
   > **I think you should be clear about which exception could cause this?** This is a key question. @kezhenxu94 showed concerns in the issue, which should be answered before any code change.
   
   Ok, I continue to look at this part of the source code and try to find the reason of exception.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102497505

   A reminder, you still don't update change log.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have exception

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102239723

   I think you should be clear about which exception could cause this? And you should follow the PR template, linking to issue and update change log.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] yangyiweigege closed pull request #8909: fix ES flush thread will stop work when flush schedule task have exception(#8852)

Posted by GitBox <gi...@apache.org>.

yangyiweigege closed pull request #8909: fix ES flush thread will stop work when flush schedule task have exception(#8852)
URL: https://github.com/apache/skywalking/pull/8909


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102284597

   Change log is a file, this link guides you to there
   
   <img width="248" alt="image" src="https://user-images.githubusercontent.com/5441976/163959773-58b5f4ae-54b4-48a1-b56c-863df43261f1.png">
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102370846

   Thanks, that is important to know why before changing. Your another bug report seems more solid, you could submit another pull request to fix that first.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] yangyiweigege commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

yangyiweigege commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102282411

   > I think you should be clear about which exception could cause this? And you should follow the PR template, linking to issue and update change log.
   
   i have changed the title and  linking to issue and update change log.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] yangyiweigege commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

yangyiweigege commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102461522

   i have found the reason of the exception:
   
   code in org.apache.skywalking.library.elasticsearch.bulk.BulkProcessor#doFlush:
   ```java
     if (batch.isEmpty()) {
               return CompletableFuture.completedFuture(null);
           }
   
           final CompletableFuture<Void> future = es.get().version().thenCompose(v -> {
               try {
                   final RequestFactory rf = v.requestFactory();
                   final List<byte[]> bs = new ArrayList<>();
                   for (final Holder holder : batch) {
                       bs.add(v.codec().encode(holder.request));
                       bs.add("\n".getBytes());
                   }
                   final ByteBuf content = Unpooled.wrappedBuffer(bs.toArray(new byte[0][]));
                   return es.get().client().execute(rf.bulk().bulk(content))
                            .aggregate().thenAccept(response -> {
                           final HttpStatus status = response.status();
                           if (status != HttpStatus.OK) {
                               throw new RuntimeException(response.contentUtf8());
                           }
                       });
               } catch (Exception e) {
                   return Exceptions.throwUnsafely(e);
               }
           });
           future.whenComplete((ignored, exception) -> {
               if (exception != null) {
                   batch.stream().map(it -> it.future)
                        .forEach(it -> it.completeExceptionally(exception));
                   log.error("Failed to execute requests in bulk", exception);
               } else {
                   log.debug("Succeeded to execute {} requests in bulk", batch.size());
                   batch.stream().map(it -> it.future).forEach(it -> it.complete(null));
               }
           });
   ```
   in this code, when response is not ok,will throw a  runtimeException,then  see  another code:
   org.apache.skywalking.library.elasticsearch.bulk.BulkProcessor#flush
   ```java
           final CompletableFuture<Void> flush = doFlush(batch);
           flush.whenComplete((ignored1, ignored2) -> semaphore.release());
           flush.join();
   ```
   
   when CompletableFuture have exception,and did't  deal it ,  the join  method  will  throw exception。 so the flush schedule will stoped .
   and i test the join method in demo:
   <img width="1223" alt="image" src="https://user-images.githubusercontent.com/23202824/163982216-fa8dea87-7e55-4c03-92c1-658c7d37007e.png">
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8909: fix ES flush thread will stop work when flush schedule task have ES connection exception

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on PR #8909:
URL: https://github.com/apache/skywalking/pull/8909#issuecomment-1102496926

   @kezhenxu94 Please confirm.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org