ctubbsii commented on PR #3189: URL: https://github.com/apache/accumulo/pull/3189#issuecomment-1422252698
> > 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](https://github.com/apache/accumulo/blob/main/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java#L340), then that will break the [client](https://github.com/apache/accumulo/blob/main/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java#L409) 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. -- 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-unsubscr...@accumulo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
