Modify ServiceLock to use ServiceLockData vs custom byte array #3189

dlmarion · 2023-02-07T14:40:04Z

This change elevates the group argument that was found in ScanServerOpts to ServerOpts, the options used as the base for all Accumulo server processes. The ScanServer used a different format for the lock data in ZooKeeper that comprised the host address and the group name, which allowed clients to find ScanServers that were part of a specific group. By moving the group from ScanServerOpts to ServerOpts it became necessary to embed the group name into the ZooKeeper lock data for all processes. Prior to this commit the ZooKeeper lock data was not using a defined structure, but a String that could be set to anything. I renamed the ServerServices class to ServiceLockData and created a well-defined structure for registering the Thrift Services that were exposed by the Accumulo server process to include the host, port, and group in this structure. Then, I modiified ServiceLock so that it expected and returned a ServiceLockData object instead of a byte[].

Closes #3178

ctubbsii

I like the changes to the ServiceLockData abstraction. I think that's a useful change on its own. After that change is done, I think the changes to add the service group are probably relatively minimal, so it's probably okay (meaning, I think I'd be in favor), but I'd like to see what this PR looks like after the ServiceLockData abstraction is done first, without the addition of the group. Would you be willing to do that?

core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java

ctubbsii · 2023-02-07T19:29:20Z

core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java

+      log.trace("tid={} Looking up manager location in zookeeper at {}.",
+          Thread.currentThread().getId(), zLockManagerPath);


Some of these log message changes seem unrelated to this PR. Might be useful to do those separately, first, so they don't detract from reviews on the main changes intended in this PR.

core/src/main/java/org/apache/accumulo/core/util/ServiceLockData.java

ctubbsii · 2023-02-07T19:47:03Z

core/src/main/java/org/apache/accumulo/core/util/ServiceLockData.java

+    this(new ServiceDescriptors(new HashSet<>(
+        Collections.singleton(new ServiceDescriptor(uuid, service, address, group)))));


Suggested change

this(new ServiceDescriptors(new HashSet<>(

Collections.singleton(new ServiceDescriptor(uuid, service, address, group)))));

this(new ServiceDescriptors(Set.of(new ServiceDescriptor(uuid, service, address, group))));

minicluster/src/test/resources/log4j2-test.properties

server/base/src/main/java/org/apache/accumulo/server/ServerOpts.java

EdColeman

This will likely make debugging test failures harder - why the change? You can always set the level in your local copy, so the question becomes who is more inconvenienced by wanting to have the level at info vs debug.

ctubbsii · 2023-02-07T20:22:09Z

This will likely make debugging test failures harder - why the change? You can always set the level in your local copy, so the question becomes who is more inconvenienced by wanting to have the level at info vs debug.

The current levels haven't been a problem at all, as far as I'm aware. This PR should probably just keep the same value that's there now so there's no risk of bloating the logs and spamming us or filling disks on CI servers. If there's a compelling reason to change it, we can definitely change it, but it hasn't been a problem to this point and it doesn't seem to be related to this PR (at least, it wasn't clear whether it was related). If Dave found it compelling to lower it to debug for testing this PR, or if we end up needing to do that a lot, that's probably a compelling reason to change it... as long as it's not too spammy. It just wasn't clear to me why it was being done here.

dlmarion · 2023-02-07T22:07:17Z

I like the changes to the ServiceLockData abstraction. I think that's a useful change on its own. After that change is done, I think the changes to add the service group are probably relatively minimal, so it's probably okay (meaning, I think I'd be in favor), but I'd like to see what this PR looks like after the ServiceLockData abstraction is done first, without the addition of the group. Would you be willing to do that?

Yes, I would be willing to do that. However, if the ServiceLockData object doesn't support the group, then that will break the client and likely break the build.

keith-turner · 2023-02-08T00:43:09Z

server/base/src/main/java/org/apache/accumulo/server/ServerOpts.java

@@ -27,6 +28,17 @@ public class ServerOpts extends ConfigOpts {
  @Parameter(names = {"-a", "--address"}, description = "address to bind to")
  private String address = null;

+  @Parameter(required = false, names = {"-g", "--group"},


Adding this option to server types where it does not do anything seems confusing to me. Like I can start a manager with the option, but it will not have any effect. Scoping it to only the tserver and scan server seems less confusing to me.

My comment above regarding using our normal configuration property would make this a non-issue. Only server types where it makes sense would have that configuration property.

ctubbsii · 2023-02-08T08:59:40Z

I like the changes to the ServiceLockData abstraction. I think that's a useful change on its own. After that change is done, I think the changes to add the service group are probably relatively minimal, so it's probably okay (meaning, I think I'd be in favor), but I'd like to see what this PR looks like after the ServiceLockData abstraction is done first, without the addition of the group. Would you be willing to do that?

Yes, I would be willing to do that. However, if the ServiceLockData object doesn't support the group, then that will break the client and likely break the build.

I am imagining the ScanServer's current -g option would go in the second PR, along with adding support for groups for tserver as well, so nothing is broken. I just think it makes sense to apply the "ServiceLockData abstraction/serialization" change feature first, then the "replace scan server group option with group added to ServiceLockData" feature after that, because they seem like they are discrete changes. I don't think the build needs to break to separate those out.

dlmarion · 2023-02-08T14:41:35Z

Considering the information about the configuration, I think I'll work on a PR to get rid of server process arguments, and replace them with properties. Then, once that is merged in, I can update this PR which should just be the ServiceLockData changes.

dlmarion · 2023-02-10T18:12:02Z

Created #3192 to remove arguments from the server processes

EdColeman · 2023-02-13T15:20:28Z

core/src/main/java/org/apache/accumulo/core/util/ServiceLockData.java

+    return toString().compareTo(other.toString());
+  }
+
+  public static Optional<ServiceLockData> parse(String lockData) {


Would this be more general if it took byte[] and did the string conversion internally to the method?

EdColeman · 2023-02-13T15:28:29Z

I general this looks like a good change. It may help if the internal representations of the lock data were more encapsulated so that the storage type was not visible outside of the LockData.

Looking at how to handle tservers and other processes that have locks when an upgrade occurs, one improvement could be if the lock data included either the data version or maybe the release version. While added either of those at this point would be out of scope for this PR, designing it so that it is easier to extend could help limit changes in the future.

dlmarion · 2023-02-13T19:14:46Z

Full IT build passed

dlmarion · 2023-02-13T19:18:50Z

It may help if the internal representations of the lock data were more encapsulated so that the storage type was not visible outside of the LockData

Can you be more specific about what you think should be changed? The storage type is a JSON string. The only serialization / deserialization that happens is inside of ServiceLockData.

EdColeman · 2023-02-13T20:21:02Z

What caught my eye was ServiceLockData.parse(new String(data, UTF_8)); and was wondering if passing a byte[] would make it possible to isolate the internals more and then the rest was speculation on potential mods that could be able to use information stored within the lock itself.

dlmarion · 2023-02-14T12:19:02Z

Another Full IT build passed. I believe I resolved all issues. I'm going to merge this morning.

dlmarion self-assigned this Feb 7, 2023

ctubbsii mentioned this pull request Feb 7, 2023

Support balancing tablets using named tserver groups #3178

Closed

ctubbsii requested changes Feb 7, 2023

View reviewed changes

EdColeman reviewed Feb 7, 2023

View reviewed changes

keith-turner reviewed Feb 8, 2023

View reviewed changes

dlmarion added 8 commits February 11, 2023 13:54

wip

85cbf43

wip

19d81ea

wip continued

ed82ed5

wip

2c2895e

use correct junit api

b7fb10e

revert log4j-test.properties changes

88c5ba8

Fix NPE happening in ExternalCompaction ITs

51fcd80

Merge in main, updates

bb05e41

dlmarion force-pushed the 3178-tserver-groups-3.0 branch from f464cfc to bb05e41 Compare February 11, 2023 16:32

dlmarion added 4 commits February 11, 2023 16:41

Fix static import

d2d46c4

Fix issue in ServiceLockIT

772ab7f

Fix tests

3a41093

Resolve findbugs issue

aca76ca

EdColeman reviewed Feb 13, 2023

View reviewed changes

Modified ServiceLockData parse/serialize to take/return byte[]

9ed2faa

EdColeman approved these changes Feb 13, 2023

View reviewed changes

dlmarion added 2 commits February 14, 2023 13:19

Addressed small suggestions to ServiceLockData

17df5d6

Add brackets to generated code

a8e845b

dlmarion merged commit 476ebf2 into apache:main Feb 14, 2023

dlmarion deleted the 3178-tserver-groups-3.0 branch February 14, 2023 13:49

dlmarion changed the title ~~Add group argument to server processes~~ Modify ServiceLock to use ServiceLockData vs custom byte array Feb 14, 2023

ctubbsii mentioned this pull request Sep 28, 2023

Compactor fails to start #3782

Closed

ctubbsii added this to the 3.0.0 milestone Jul 12, 2024

ctubbsii mentioned this pull request Oct 12, 2024

Make it easier to know how to set the advertised address for a server process #4976

Closed

		log.trace("tid={} Looking up manager location in zookeeper at {}.",
		Thread.currentThread().getId(), zLockManagerPath);

		this(new ServiceDescriptors(new HashSet<>(
		Collections.singleton(new ServiceDescriptor(uuid, service, address, group)))));

	this(new ServiceDescriptors(new HashSet<>(
	Collections.singleton(new ServiceDescriptor(uuid, service, address, group)))));
	this(new ServiceDescriptors(Set.of(new ServiceDescriptor(uuid, service, address, group))));

Modify ServiceLock to use ServiceLockData vs custom byte array #3189

Modify ServiceLock to use ServiceLockData vs custom byte array #3189

Uh oh!

Conversation

dlmarion commented Feb 7, 2023

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ctubbsii Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ctubbsii Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EdColeman left a comment

Choose a reason for hiding this comment

Uh oh!

ctubbsii commented Feb 7, 2023

Uh oh!

dlmarion commented Feb 7, 2023

Uh oh!

keith-turner Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

ctubbsii Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

ctubbsii commented Feb 8, 2023

Uh oh!

dlmarion commented Feb 8, 2023

Uh oh!

dlmarion commented Feb 10, 2023

Uh oh!

EdColeman Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

EdColeman commented Feb 13, 2023

Uh oh!

dlmarion commented Feb 13, 2023

Uh oh!

dlmarion commented Feb 13, 2023

Uh oh!

EdColeman commented Feb 13, 2023

Uh oh!

dlmarion commented Feb 14, 2023

Uh oh!

Uh oh!