You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/01/28 03:07:06 UTC
[GitHub] [druid] clintropolis opened a new pull request #12209: remove duplicate Broker ServerInventoryView, improve HttpServerInventoryView logging
clintropolis opened a new pull request #12209:
URL: https://github.com/apache/druid/pull/12209
### Description
This PR adds two improvements to brokers. The first, is that `SystemSchema` was causing a second `ServerInventoryView` to be created to power the "servers" table, due to injecting `InventoryView`, which due to bindings uses a `ServerInventoryView` from the `ServerInventoryViewProvider`, while everything else on the broker uses `FilteredServerInventoryView` (including the `TimelineServerView` used by the "server segments" table which on brokers is `BrokerServerView` which uses `FilteredServerInventoryView`).
The "servers" table is only using the `InventoryView` to get `DruidServer`, which are not filtered afaict by any of the `FilteredServerInventoryView` implementations anyway, so it is safe to change `SystemSchema` to use this instead (and it is also more consistent with the "server segments" table to use the same inventory view).
Prior to this PR, during broker startup you would see
```
2022-01-28T01:39:45,069 INFO [main] org.apache.druid.java.util.emitter.core.LoggingEmitter - Start: started [true]
2022-01-28T01:39:45,088 INFO [main] org.apache.druid.client.HttpServerInventoryView - Starting FilteredHttpServerInventoryView.
...
2022-01-28T01:39:51,529 INFO [main] org.apache.druid.sql.calcite.schema.MetadataSegmentView - MetadataSegmentView Started.
2022-01-28T01:39:51,530 INFO [main] org.apache.druid.client.HttpServerInventoryView - Starting HttpServerInventoryView.
...
```
while after now only the `FilteredHttpServerInventoryView` can be observed to be started.
This might help reduce broker memory footprints by quite a bit since there won't be double the segment tracking happening! I believe this issue has existed since #7706.
The second change reduces the log verbosity when using the `HttpServerInventoryView` when it runs into any of the scenarios where it needs to perform a 'full sync' on a data server, which can result in "duplicate" segments getting added which causes a flurry of log "warning" messages (which are expected in this case). These log warnings are now suppressed when performing a full sync (but will still warn if it happens during a delta, where duplicates _are_ unexpected).
It was investigation of the latter issue which caused me to stumble into the former, where while debugging I noticed that I had completely separate inventory views running at the same time in my broker.
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #12209: remove duplicate Broker ServerInventoryView, improve HttpServerInventoryView logging
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #12209:
URL: https://github.com/apache/druid/pull/12209#issuecomment-1029394457
gave up on getting coverage improved (since the lines not covered are logs)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #12209: remove duplicate Broker ServerInventoryView, improve HttpServerInventoryView logging
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #12209:
URL: https://github.com/apache/druid/pull/12209
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis edited a comment on pull request #12209: remove duplicate Broker ServerInventoryView, improve HttpServerInventoryView logging
Posted by GitBox <gi...@apache.org>.
clintropolis edited a comment on pull request #12209:
URL: https://github.com/apache/druid/pull/12209#issuecomment-1023976246
still trying to figure out how to get adequate coverage on `HttpServerInventoryView` to make the bot happy, in the meantime I've manually triggered the integration tests to make sure everything is otherwise still working correctly
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #12209: remove duplicate Broker ServerInventoryView, improve HttpServerInventoryView logging
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #12209:
URL: https://github.com/apache/druid/pull/12209#issuecomment-1023976246
still trying to figure out how to get adequate coverage on `HttpServerInventoryView` to make the bot happy
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org