Add system properties table

[GabrielCWT]

This PR adds a new properties table which will call the /status/properties endpoint of each of the nodes.

The feature allows for an aggregated view of the properties of all services.

[kfaraz]

Thanks for the changes, @GabrielCWT !
I have often found myself needing this.
Overall approach makes sense to me.
Please add an embedded test for this feature (see IngestionSmokeTest for an example).

[GabrielCWT]

Hi @kfaraz, when implementing the embedded test, I encountered a strange bug which I could not pinpoint the issue. I'm hoping you have more insight on this issue.

When sending the /status/properties request to the coordinator's localhost:8081, it would return the broker's properties. I confirmed that the coordinator was initialising it's own StatusResource and Properties object.

It seems like the coordinator is hitting the broker's endpoint even though I am using localhost:8081.

When I added a router server, the coordinator would return the router's properties instead.

  protected EmbeddedDruidCluster createCluster()
  {
    return EmbeddedDruidCluster
        .withZookeeper()
        .addServer(new EmbeddedCoordinator())
        .addServer(overlord)
        .addServer(broker)
        .addServer(new EmbeddedRouter()) // added here
        .addCommonProperty("commonProperty", "commonValue");
  }

Note: Currently I have omitted verifying the coordinator's properties in the embedded test for the reason above.

[kfaraz]

Thanks for the update, @GabrielCWT . I will take a look at the test.

[kfaraz]

Left some more comments.

@GabrielCWT , to fix the test, please
change this line

druid/services/src/main/java/org/apache/druid/cli/CoordinatorJettyServerInitializer.java

Lines 112 to 113 in 4de1909

	final FilterHolder guiceFilterHolder = JettyServerInitUtils.getGuiceFilterHolder(injector);
	root.addFilter(GuiceFilter.class, "/status/*", null);

to be the following instead:

final FilterHolder guiceFilterHolder = JettyServerInitUtils.getGuiceFilterHolder(injector);
root.addFilter(guiceFilterHolder, "/status/*", null);

I must have missed this one while fixing up the rest for the embedded test framework.

[FrankChen021]

LGTM with a minor suggestion

[abhishekrb19]

Could we call this ServerPropertiesTable to be clearer? (similar to ServerSegmentsTable for server_segments table)

[GabrielCWT]

Will rename it to SystemServerPropertiesTable instead to maintain the idea that it is a system table

[abhishekrb19]

This is cool! The hidden or secret properties on each server wouldn’t be listed in this systems table as it goes through the /status/properties, right? I think it would be good to add a test for that. Could you also please update the docs for this table here https://druid.apache.org/docs/latest/querying/sql-metadata-tables/#system-schema?

[kfaraz]

Left some minor suggestions. Changes look good.

[kfaraz]

We should also add the test suggested by @abhishekrb19 , but that is not a blocker for this PR and can be done in a follow up.

Thanks for the changes, @GabrielCWT !

[GabrielCWT]

We should also add the test suggested by @abhishekrb19 , but that is not a blocker for this PR and can be done in a follow up.

@kfaraz I've added a minimal test to check if hidden properties are exposed.

Also do we still need tests in SystemSchemaTest since we implemented the tests in SystemServerPropertiesTableTest?

[kfaraz]

@kfaraz I've added a minimal test to check if hidden properties are exposed.

Thanks, @GabrielCWT !

Also do we still need tests in SystemSchemaTest since we implemented the tests in SystemServerPropertiesTableTest?

Yes, since you have already added those UTs, let's keep them.
Embedded tests are primarily for integration testing.
Method/class level functional testing should still be done in UTs.

[kfaraz]

@FrankChen021 , @GabrielCWT , are any further changes pending in this PR?

[GabrielCWT]

No other changes @kfaraz