You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2024/12/15 17:39:41 UTC
[PR] Add Mergeability column to support automatic merges [accumulo]
cshannon opened a new pull request, #5187:
URL: https://github.com/apache/accumulo/pull/5187
This adds a new Mergeability column for marking if/when tablets are eligible to me merged by the system based on a threshold. The column stores a duration that is relative to the time the Manager uses, Steady time.
There are 3 possible states for the value:
1) -1 : This means a tablet will never automatically merge
2) 0 : Tablet is eligible now to automatically merge
3) positive duration: eligible to merge after the given duration relative
to the current system Steady time. Ie. the tablet can be merged if SteadyTime is later than the given duration.
This change only adds the new column itself and populates it. The default is to never merge automatically for all cases except for when the system automatically splits tablets. In that case the newly split tablets are marked as being eligible to merge immediately.
Future updates will add API enhancements to allow setting/viewing the mergeability setting as well as to enable automatic merging by the system that is based on this new column value.
When automatic merging is enabled, if a user wants to make a tablet elgible to be merged in the future they would do so by adding a period of time to the current SteadyTime. For example, to make a tablet eligible to be merged 3 days from now the user would read the current SteadyTime value (represented as a duration), add 3 days to that and then store that new duration in the column. When the current steady time passes that duration the tablet would be eligible to be merged.
See #5014 for more details
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910313384
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
+ return delay.toNanos() > 0;
+ }
+
+ public Duration getDelay() {
+ return delay;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ TabletMergeability that = (TabletMergeability) o;
+ return Objects.equals(delay, that.delay);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(delay);
+ }
+
+ @Override
+ public String toString() {
+ if (isNow()) {
+ return "TabletMergeability=NOW";
+ } else if (isNever()) {
+ return "TabletMergeability=NEVER";
+ }
+ return "TabletMergeability=AFTER:" + delay.toNanos();
+ }
+
+ public static TabletMergeability from(Duration delay) {
+ Preconditions.checkArgument(delay.toNanos() >= -1,
+ "Duration of delay must be -1, 0, or a positive offset.");
+ return new TabletMergeability(delay);
+ }
Review Comment:
Also I will add some javadocs and we can tweak them in future PRs if things evolve/change which should be fine as long as we do it before we release 4.0.0
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1909064640
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
Review Comment:
Need `@since` tags in javadoc since this class is in a public API package.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
Review Comment:
Feel free to ignore this suggestion. Was thinking this name went better w/ the getDelay() method.
```suggestion
public boolean isDelayed() {
```
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
+ return delay.toNanos() > 0;
+ }
+
+ public Duration getDelay() {
+ return delay;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ TabletMergeability that = (TabletMergeability) o;
+ return Objects.equals(delay, that.delay);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(delay);
+ }
+
+ @Override
+ public String toString() {
+ if (isNow()) {
+ return "TabletMergeability=NOW";
+ } else if (isNever()) {
+ return "TabletMergeability=NEVER";
+ }
+ return "TabletMergeability=AFTER:" + delay.toNanos();
+ }
+
+ public static TabletMergeability from(Duration delay) {
+ Preconditions.checkArgument(delay.toNanos() >= -1,
+ "Duration of delay must be -1, 0, or a positive offset.");
+ return new TabletMergeability(delay);
+ }
Review Comment:
This method needs javadoc outlining the acceptable input values for duration. Since this is a public API class wondering if the following would be better instead, however those methods would still need javadoc. Could make those constants private if adding these also, thinking the methods give a bit more flexibility to change.
```suggestion
public static TabletMergeability now(){
return NOW:
}
public static TabletMergeability never(){
return NEVER:
}
```
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
+ return delay.toNanos() > 0;
+ }
+
+ public Duration getDelay() {
+ return delay;
Review Comment:
Could do the following to give more flexibility w/ changing how now and never are represented, would need add javadoc if doing so.
```suggestion
public Duration getDelay() {
Preconditions.checkstate(isDelayed());
return delay;
```
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ofNanos(0));
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public Duration getDelay() {
+ return delay;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ TabletMergeability that = (TabletMergeability) o;
+ return Objects.equals(delay, that.delay);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(delay);
+ }
+
+ @Override
+ public String toString() {
+ if (isNow()) {
+ return "TabletMergeability=NOW";
+ } else if (isNever()) {
+ return "TabletMergeability=NEVER";
+ }
+ return "TabletMergeability=AFTER:" + delay.toNanos();
Review Comment:
Could convert to ms and include the unit at the end of the string.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910311570
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
Review Comment:
I think isDelayed() is probably better and will rename it.
I kept going back and forth with trying to decide on the correct names and I think this may change anyways. I've been wondering if instead of `NOW` it should be `ALWAYS` or `IMMEDIATE` etc as those names could align better with NEVER. I will probably leave the name alone for now because I think the exact naming is going to be how we intent to use this going forward and if we want to rename it and TabletMergeability itself could possibly be renamed in a future PR.
Right now the name is TabletMereability but as @ctubbsii had pointed out in a previous conversation, the current goal for this is really tablet _automatic_ mergeability so we can use it to determine if we can run an automatic merge on a tablet range just like we do automatic splits.
However, there has been some talk of making it more generic to mergeabilty in general (Even regular user triggered merges) which may also impact how we name things but not sure we will or need to. Either way, I expect in future PRs as this evolves the naming is going to be tweaked a bit.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#issuecomment-2583452784
I'm going to go ahead and merge this because there will be follow on PRs anyways so those can address changes going forward (which I'm sure there will be) as this feature evolves with the client API and the Fate code for handling automatic merging. I will be working on the client API changes next for setting the mergeability value of a table and tablets.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1887716359
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ofNanos(0));
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public Duration getDelay() {
+ return delay;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ TabletMergeability that = (TabletMergeability) o;
+ return Objects.equals(delay, that.delay);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(delay);
+ }
+
+ @Override
+ public String toString() {
+ if (isNow()) {
+ return "TabletMergeability=NOW";
+ } else if (isNever()) {
+ return "TabletMergeability=NEVER";
+ }
+ return "TabletMergeability=AFTER:" + delay.toNanos();
Review Comment:
Yeah, not a bad idea, nanoseconds would be pretty tough to understand in a log.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910523172
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
Review Comment:
This is an interesting idea, I think that makes sense to allow a delay of >= 0 because it fits in nicely as part of the regular delay logic but NEVER is the edge case as you said. For the json encoding we keep the boolean and long but inside the TabletMergeability class maybe we can just represent "NEVER" with a null delay. I think we should still keep the always() method shortcut I think because it's going to be a commonly used value.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910404398
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
+ return delay.toNanos() > 0;
+ }
+
+ public Duration getDelay() {
+ return delay;
Review Comment:
I didn't end up making this change yet because it would make things a bit tricky for things like json serialization. It would make the serialization harder because the json code would now have to duplicate business logic and know to translate NEVER to -1 and NOW to 0 for the delay. [This](https://github.com/apache/accumulo/pull/5187/files#diff-d0c46782a2fa101a9e218b4e37c26a18c066e0745128fb351b08b4e8f1480483R84) is the spot I am talking about.
That code would have to make sure to not call getDelay() unless it was positive and we would introduce multiple locations in the code that need to be updated if we wanted to change the meaning of -1 and 0. They are different packages so we can't make it package scope for the private delay either.
All that being said, I think this is doable and not a bad idea but we if we do this I think it probably makes sense to completely decouple the serialization as well if the intent is to be more flexible in the future and no other code should really know what -1 and 0 mean outside of the internals of TabletMergeability. This would mean only allowing publicly accessible methods that create a TabletMergeability to set a delay of > 0 and requiring the use of TabletMergeability.now() or TabletMergeability.never() for all other cases so [this](https://github.com/apache/accumulo/pull/5187/files#diff-b7655aae54212259b0644ce511c60e22e39b1f9122701f427a176587477921b3R141) method would go away.
For serialization, I think the json format in TabletMergeabilityMetadata would need to be changed to something like the following:
```java
private static class GSonData {
boolean never;
boolean now;
Long delay;
Long steadyTime;
}
```
and we would just have to set steadyTime and delay to null values if either never or now was true.
@keith-turner - thoughts?
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910410525
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
+ return delay.toNanos() > 0;
+ }
+
+ public Duration getDelay() {
+ return delay;
Review Comment:
Taking this one step further you could even just require the delay in TabletMergeability to always be positive and never < 0 and add a couple boolean flags to signal NOW and NEVER and just make the delay null in that case vs using 0 or -1. It probably doesn't matter either way though if we keep using 0 and -1 as it would be hidden internally to the class and not exposed.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1887352174
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ofNanos(0));
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public Duration getDelay() {
+ return delay;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ TabletMergeability that = (TabletMergeability) o;
+ return Objects.equals(delay, that.delay);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(delay);
+ }
+
+ @Override
+ public String toString() {
+ if (isNow()) {
+ return "TabletMergeability=NOW";
+ } else if (isNever()) {
+ return "TabletMergeability=NEVER";
+ }
+ return "TabletMergeability=AFTER:" + delay.toNanos();
Review Comment:
We may want to convert this to `ms` or `s` in the `toString` method for ease of understanding.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910323391
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
Review Comment:
@keith-turner - The more I think about it, the more I'm not sure I like using `NOW` because it's a bit misleading. A tablet could be eligible to be merged now even if a delay is set if enough time has passed. So the value isn't really signaling that it's eligible now so much as that it's always eligible no matter what because the delay is 0 so I'm back to thinking maybe we rename it to something like ALWAYS or IMMEDIATELY.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910520247
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
+ return delay.toNanos() > 0;
+ }
+
+ public Duration getDelay() {
+ return delay;
Review Comment:
Now its documented on the javadoc what -1,0, and >0 mean, which seems good for now. Its nice to structure the API so that the -1 case does not cause bugs in code calling getDelay AND to make it more flexible to change, but not really sure of the best way to achieve those goals. The javadoc lets someone know hey you need to handle this -1 case, which helps w/ the usage bug case. Making the API more flexible is tricky and as you mentioned this will probably evolve as more of the feature is implemented also, so it would be good to see what is learned from that.
If the code is released w/ that javadoc then its behavior is set and it would be ok for the serialization code to be tightly coupled to the behavior in the javadoc.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1894275850
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -392,6 +393,8 @@ interface TabletUpdates<T> {
T putCloned();
+ T putTabletMergeability(TabletMergeability tabletMergeability);
Review Comment:
My original plan was to persist everything as one value by adding the current manager time to the value. So basically to compute the mergability duration you would read the current SteadyTime then add the offset to it and store it and then later you could compare to the current time by taking a diff.
However, I was thinking more about it and I think that storing it as two separate values makes sense because it allows you to do better debugging (logigng) plus you can see extra information such as when the value was created. It also allows doing other things like update/replacing the original SteadyTime, etc and other metric calculations if we keep it separate so I'll change it.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon merged PR #5187:
URL: https://github.com/apache/accumulo/pull/5187
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910311570
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
Review Comment:
I think isDelayed() is probably better and will rename it.
I kept going back and forth with trying to decide on the correct names and I think this may change anyways. I've been wondering if instead of `NOW` it should be `ALWAYS` as ALWAYS aligns better with NEVER. I will probably leave the name alone for now because I think the exact naming is going to be how we intent to use this going forward and if we want to rename it and TabletMergeability itself could possibly be renamed in a future PR.
Right now the name is TabletMereability but as @ctubbsii had pointed out in a previous conversation, the current goal for this is really tablet _automatic_ mergeability so we can use it to determine if we can run an automatic merge on a tablet range just like we do automatic splits.
However, there has been some talk of making it more generic to mergeabilty in general (Even regular user triggered merges) but not sure we will or need to. Either way, I expect in future PRs as this evolves the naming is going to be tweaked a bit.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1903121181
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -392,6 +393,8 @@ interface TabletUpdates<T> {
T putCloned();
+ T putTabletMergeability(TabletMergeability tabletMergeability);
Review Comment:
@keith-turner - The latest update [adds](https://github.com/apache/accumulo/pull/5187/files#diff-d0c46782a2fa101a9e218b4e37c26a18c066e0745128fb351b08b4e8f1480483R35) a new class just to wrap the metadata and make it easier to validate and store the delay + the steady time as a separate value. The intent is that the the client will still only use and pass [TabletMergeability](https://github.com/apache/accumulo/pull/5187/files#diff-b7655aae54212259b0644ce511c60e22e39b1f9122701f427a176587477921b3R27) because only the Manager will be touching `TabletMergeabilityMetadata` as it just exists in Ample. A user would just set the `TabletMergeability` and if a delay is set then the Manager will set the SteadyTime value when inserting `TabletMergeabilityMetadata`.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910404398
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
+ return delay.toNanos() > 0;
+ }
+
+ public Duration getDelay() {
+ return delay;
Review Comment:
I didn't end up making this change yet because it would make things a bit tricky for things like json serialization. It would make the serialization harder because the json code would now have to duplicate business logic and know to translate NEVER to -1 and NOW to 0 for the delay. [This](https://github.com/apache/accumulo/pull/5187/files#diff-d0c46782a2fa101a9e218b4e37c26a18c066e0745128fb351b08b4e8f1480483R84) is the spot I am talking about.
That code would have to make sure to not call getDelay() unless it was positive and we would introduce multiple locations in the code that need to be updated if we wanted to change the meaning of -1 and 0. They are different packages so we can't make it package scope for the private delay either.
All that being said, I think this is doable and not a bad idea but if we do this I think it probably makes sense to completely decouple the serialization as well if the intent is to be more flexible in the future and no other code should really know what -1 and 0 mean outside of the internals of TabletMergeability. This would mean only allowing publicly accessible methods that create a TabletMergeability to set a delay of > 0 and requiring the use of TabletMergeability.now() or TabletMergeability.never() for all other cases so [this](https://github.com/apache/accumulo/pull/5187/files#diff-b7655aae54212259b0644ce511c60e22e39b1f9122701f427a176587477921b3R141) method would go away.
For serialization, I think the json format in TabletMergeabilityMetadata would need to be changed to something like the following:
```java
private static class GSonData {
boolean never;
boolean now;
Long delay;
Long steadyTime;
}
```
and we would just have to set steadyTime and delay to null values if either never or now was true.
@keith-turner - thoughts?
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1894421928
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -392,6 +393,8 @@ interface TabletUpdates<T> {
T putCloned();
+ T putTabletMergeability(TabletMergeability tabletMergeability);
Review Comment:
> My original plan was to persist everything as one value by adding the current manager time to the value.
Ok I had thought this would store the exact duration specified by the user. Summing the steady time and duration from the user would work. There may be a slight advantage for keeping them separate for debugging purposes as mentioned.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910285966
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
+ return delay.toNanos() > 0;
+ }
+
+ public Duration getDelay() {
+ return delay;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ TabletMergeability that = (TabletMergeability) o;
+ return Objects.equals(delay, that.delay);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(delay);
+ }
+
+ @Override
+ public String toString() {
+ if (isNow()) {
+ return "TabletMergeability=NOW";
+ } else if (isNever()) {
+ return "TabletMergeability=NEVER";
+ }
+ return "TabletMergeability=AFTER:" + delay.toNanos();
+ }
+
+ public static TabletMergeability from(Duration delay) {
+ Preconditions.checkArgument(delay.toNanos() >= -1,
+ "Duration of delay must be -1, 0, or a positive offset.");
+ return new TabletMergeability(delay);
+ }
Review Comment:
This is a good idea and I may do this as well in `TabletMergeabilityMetadata`. The reason is I have been thinking that the NOW, NEVER etc constants that I defined in TabletMergeability and TabletMergeabilityMetadata could lead to future mistakes if developers forget they are not enums and incorrectly use an identity check for equality. At first glance `TabletMergeability.NOW` looks like an enum value when it isn't. In theory if the code always uses the constant then an identity check for equality would be true when checked, but it would still be safer and more correct to use .equals() and not == since it isn't an enum and both TabletMergeability and TabletMergeabilityMetadata have equals/hashcode defined.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910311570
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
Review Comment:
I think isDelayed() is probably better and will rename it.
I kept going back and forth with trying to decide on the correct names and I think this may change anyways. I've been wondering if instead of `NOW` it should be `ALWAYS` as ALWAYS aligns better with NEVER. I will probably leave the name alone for now because I think the exact naming is going to be how we intent to use this going forward and if we want to rename it and TabletMergeability itself could possibly be renamed in a future PR.
Right now the name is TabletMereability but as @ctubbsii had pointed out in a previous conversation, the current goal for this is really tablet _automatic_ mergeability so we can use it to determine if we can run an automatic merge on a tablet range just like we do automatic splits.
However, there has been some talk of making it more generic to mergeabilty in general (Even regular user triggered merges) which may also impact how we name things but not sure we will or need to. Either way, I expect in future PRs as this evolves the naming is going to be tweaked a bit.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1910505385
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
+
+ private final Duration delay;
+
+ private TabletMergeability(Duration delay) {
+ this.delay = Objects.requireNonNull(delay);
+ }
+
+ public boolean isNever() {
+ return this.delay.isNegative();
+ }
+
+ public boolean isNow() {
+ return this.delay.isZero();
+ }
+
+ public boolean isFuture() {
Review Comment:
Oh yeah NOW is kinda tricky. I like ALWAYS. Maybe could have two states instead of three by making delay be >=0 instead of >0. Then its just NEVER or DELAYED. NEVER is the one that is nice to specifically designate intention. From an encoding standpoint that would be a boolean and a long.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1894217008
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TabletMergeability.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.util.Objects;
+
+import com.google.common.base.Preconditions;
+
+public class TabletMergeability implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ public static final TabletMergeability NEVER = new TabletMergeability(Duration.ofNanos(-1));
+ public static final TabletMergeability NOW = new TabletMergeability(Duration.ofNanos(0));
Review Comment:
```suggestion
public static final TabletMergeability NOW = new TabletMergeability(Duration.ZERO);
```
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -392,6 +393,8 @@ interface TabletUpdates<T> {
T putCloned();
+ T putTabletMergeability(TabletMergeability tabletMergeability);
Review Comment:
Eventually we will need a steady time to evaluate if something should merge. Seems like the steady time should always be set when the mergability is set. Although its only needed when the duration is > 0. The steady time could be encoded in the same columns value.
```suggestion
T putTabletMergeability(TabletMergeability tabletMergeability, SteadyTime steadTime);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/merge/MergeTablets.java:
##########
@@ -187,6 +187,20 @@ public Repo<Manager> call(FateId fateId, Manager manager) throws Exception {
DeleteRows.getMergeTabletAvailability(range, tabletAvailabilities));
tabletMutator.putPrevEndRow(firstTabletMeta.getPrevEndRow());
+ // TODO: How should we determine this value this after merging?
+ // Do we just keep the value that's already in the last tablet in the range?
Review Comment:
Keeping the one on the last tablet as is makes sense if we think of mergability being set on a split point. So some mergabiity was set on the last split in the past, so need to keep it. The other split points are being merged away, so no longer care about their mergability.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add Mergeability column to support automatic merges [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5187:
URL: https://github.com/apache/accumulo/pull/5187#discussion_r1894244093
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -392,6 +393,8 @@ interface TabletUpdates<T> {
T putCloned();
+ T putTabletMergeability(TabletMergeability tabletMergeability);
Review Comment:
Steady time could be added in later PRs. Just wondering how it will be persisted.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org