Skip to content

Conversation

@ddanielr
Copy link
Contributor

Adds the scan server properties prefix to the list of valid zooProps.
This allows the scan server properties to be modified via the shell.

Adds the scan server properties prefix to the list of valid zooProps.
This allows the scan server properties to be modified via the shell.
@ddanielr ddanielr requested a review from dlmarion April 23, 2024 12:00
Copy link
Contributor

@dlmarion dlmarion left a comment

Choose a reason for hiding this comment

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

No issue with this change. However, I think Property.fixedProperties needs to be reviewed. There are properties that we are marking as mutable by using the prefix property that require a restart, but are not in the fixedProperties set. For example, tserver.server.threads.minimum is marked mutable, so it can be changed in ZK, but the TServer will not update it's internal value and restart the Thrift Server.

@ddanielr
Copy link
Contributor Author

No issue with this change. However, I think Property.fixedProperties needs to be reviewed. There are properties that we are marking as mutable by using the prefix property that require a restart, but are not in the fixedProperties set. For example, tserver.server.threads.minimum is marked mutable, so it can be changed in ZK, but the TServer will not update it's internal value and restart the Thrift Server.

Looking at this, should all the properties that are used as arguments to TServerUtils.startServer be considered fixed?

public static ServerAddress startServer(ServerContext context, String hostname,
Property portHintProperty, TProcessor processor, String serverName, String threadName,
Property portSearchProperty, Property minThreadProperty, Property threadTimeOutProperty,
Property timeBetweenThreadChecksProperty, Property maxMessageSizeProperty)
throws UnknownHostException {

If so I can make updates to Property.fixedProperties for the various missing properties.

@dlmarion
Copy link
Contributor

Looking at this, should all the properties that are used as arguments to TServerUtils.startServer be considered fixed?

Most likely. Also, the cache properties are likely also fixed. The code would need to be evaluated to determine if a running server will respect property changes. Being ZK mutable allows the user to change the property in the shell. It will take effect for servers started after the change, but there is no guarantee that running servers will be updated.

@dlmarion
Copy link
Contributor

@ddanielr - I looked through the ScanServer code. These need to be added to the fixedProperties. The only properties that are checked at runtime are the ones dealing with the scan executors.

      // sserver fixed properties
      SSERV_CACHED_TABLET_METADATA_EXPIRATION, SSERV_PORTSEARCH, SSERV_DATACACHE_SIZE,
      SSERV_INDEXCACHE_SIZE, SSERV_SUMMARYCACHE_SIZE, SSERV_DEFAULT_BLOCKSIZE,
      SSERVER_SCAN_REFERENCE_EXPIRATION_TIME, SSERV_CLIENTPORT, SSERV_MAX_MESSAGE_SIZE,
      SSERV_MINTHREADS, SSERV_MINTHREADS_TIMEOUT, SSERV_THREADCHECK,

Updates the list of fixed properties to include properties that are used
when setting up a server and are never checked again at runtime.
@ddanielr
Copy link
Contributor Author

@ddanielr - I looked through the ScanServer code. These need to be added to the fixedProperties. The only properties that are checked at runtime are the ones dealing with the scan executors.

      // sserver fixed properties
      SSERV_CACHED_TABLET_METADATA_EXPIRATION, SSERV_PORTSEARCH, SSERV_DATACACHE_SIZE,
      SSERV_INDEXCACHE_SIZE, SSERV_SUMMARYCACHE_SIZE, SSERV_DEFAULT_BLOCKSIZE,
      SSERVER_SCAN_REFERENCE_EXPIRATION_TIME, SSERV_CLIENTPORT, SSERV_MAX_MESSAGE_SIZE,
      SSERV_MINTHREADS, SSERV_MINTHREADS_TIMEOUT, SSERV_THREADCHECK,

I took that list and also expanded it to include those properties under any other server types.

Copy link
Contributor

@dlmarion dlmarion left a comment

Choose a reason for hiding this comment

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

I think this is good. There are likely many others that are fixed, but that can be done in a different PR.

@ddanielr ddanielr merged commit 0958a5a into apache:2.1 Apr 23, 2024
@ddanielr ddanielr deleted the fix-scan-server-prefix branch April 23, 2024 17:24
@ctubbsii
Copy link
Member

It would be nice to have an extra attribute on each property that denoted these attributes, so we didn't have to maintain a separate list. I'm not sure that would be a huge improvement, but it might make it easier to keep up-to-date because we'd have to consciously think about these attributes when we create or modify properties, rather than having to add them to a separate hard-coded list somewhere else when they need to be updated.

@ctubbsii ctubbsii modified the milestones: 4.0.0, 3.1.0, 2.1.3 Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants