You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/12/05 19:05:36 UTC
[GitHub] [accumulo] dlmarion opened a new pull request, #3107: Decompose tablet thrift svcs main
dlmarion opened a new pull request, #3107:
URL: https://github.com/apache/accumulo/pull/3107
Decompose tabletserver.thrift into multiple files that contain more targeted services (scan, ingest, tablet management, and tablet server).
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3107: Decompose tablet thrift svcs main
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3107:
URL: https://github.com/apache/accumulo/pull/3107#discussion_r1044604248
##########
core/src/main/scripts/generate-thrift.sh:
##########
@@ -32,7 +32,7 @@
[[ -z $REQUIRED_THRIFT_VERSION ]] && REQUIRED_THRIFT_VERSION='0.17.0'
[[ -z $INCLUDED_MODULES ]] && INCLUDED_MODULES=()
[[ -z $BASE_OUTPUT_PACKAGE ]] && BASE_OUTPUT_PACKAGE='org.apache.accumulo.core'
-[[ -z $PACKAGES_TO_GENERATE ]] && PACKAGES_TO_GENERATE=(gc master manager tabletserver securityImpl clientImpl dataImpl trace compaction)
+[[ -z $PACKAGES_TO_GENERATE ]] && PACKAGES_TO_GENERATE=(gc master manager tabletserver securityImpl clientImpl dataImpl trace compaction tabletingest tablet tabletscan)
Review Comment:
Yes, it contains the TInfo struct.
--
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
[GitHub] [accumulo] dlmarion commented on pull request #3107: Decompose tablet thrift svcs main
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #3107:
URL: https://github.com/apache/accumulo/pull/3107#issuecomment-1361266352
@ctubbsii - did you want to review this further? I'm thinking we should merge this sooner rather than later. FWIW, I ran a full IT build last night again and it succeeded.
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3107: Decompose tablet thrift svcs main
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3107:
URL: https://github.com/apache/accumulo/pull/3107#discussion_r1044619896
##########
core/src/main/java/org/apache/accumulo/core/rpc/clients/TabletIngestClientServiceThriftClient.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.rpc.clients;
+
+import org.apache.accumulo.core.tabletingest.thrift.TabletIngestClientService.Client;
+
+public class TabletIngestClientServiceThriftClient extends ThriftClientTypes<Client> {
Review Comment:
Added javadoc in 0d0f185
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3107: Decompose tablet thrift svcs main
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3107:
URL: https://github.com/apache/accumulo/pull/3107#discussion_r1042110555
##########
core/src/main/thrift/tabletingest.thrift:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.
+ */
+namespace java org.apache.accumulo.core.tabletingest.thrift
+namespace cpp org.apache.accumulo.core.tabletingest.thrift
+
+include "data.thrift"
+include "security.thrift"
+include "client.thrift"
+include "manager.thrift"
+include "master.thrift"
+include "trace.thrift"
+include "tabletserver.thrift"
+
+exception ConstraintViolationException {
+ 1:list<data.TConstraintViolationSummary> violationSummaries
+}
+
+enum TDurability {
+ DEFAULT = 0
+ SYNC = 1
+ FLUSH = 2
+ LOG = 3
+ NONE = 4
+}
+
+service TabletIngestClientService {
Review Comment:
I just decomposed tabletserver.thrift into multiple services. I don't think anything should have been added or removed, just relocated.
--
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
[GitHub] [accumulo] dlmarion merged pull request #3107: Decompose tablet thrift svcs main
Posted by GitBox <gi...@apache.org>.
dlmarion merged PR #3107:
URL: https://github.com/apache/accumulo/pull/3107
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #3107: Decompose tablet thrift svcs main
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3107:
URL: https://github.com/apache/accumulo/pull/3107#issuecomment-1370318969
> @ctubbsii - did you want to review this further? I'm thinking we should merge this sooner rather than later. FWIW, I ran a full IT build last night again and it succeeded.
Delayed response. No. You addressed all the things I had comments on. Thanks :smiley_cat:
--
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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3107: Decompose tablet thrift svcs main
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #3107:
URL: https://github.com/apache/accumulo/pull/3107#discussion_r1044611805
##########
core/src/main/scripts/generate-thrift.sh:
##########
@@ -32,7 +32,7 @@
[[ -z $REQUIRED_THRIFT_VERSION ]] && REQUIRED_THRIFT_VERSION='0.17.0'
[[ -z $INCLUDED_MODULES ]] && INCLUDED_MODULES=()
[[ -z $BASE_OUTPUT_PACKAGE ]] && BASE_OUTPUT_PACKAGE='org.apache.accumulo.core'
-[[ -z $PACKAGES_TO_GENERATE ]] && PACKAGES_TO_GENERATE=(gc master manager tabletserver securityImpl clientImpl dataImpl trace compaction)
+[[ -z $PACKAGES_TO_GENERATE ]] && PACKAGES_TO_GENERATE=(gc master manager tabletserver securityImpl clientImpl dataImpl trace compaction tabletingest tablet tabletscan)
Review Comment:
I created https://github.com/apache/accumulo/issues/3113 to deal with this.
--
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
[GitHub] [accumulo] dlmarion commented on pull request #3107: Decompose tablet thrift svcs main
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #3107:
URL: https://github.com/apache/accumulo/pull/3107#issuecomment-1339239274
Full IT build succeeded
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3107: Decompose tablet thrift svcs main
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #3107:
URL: https://github.com/apache/accumulo/pull/3107#discussion_r1041788124
##########
core/src/main/java/org/apache/accumulo/core/rpc/clients/TabletIngestClientServiceThriftClient.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.rpc.clients;
+
+import org.apache.accumulo.core.tabletingest.thrift.TabletIngestClientService.Client;
+
+public class TabletIngestClientServiceThriftClient extends ThriftClientTypes<Client> {
Review Comment:
These names are not intuitive on their own. It would be helpful to add a javadoc to explain the scope of these services.
##########
core/src/main/thrift/tabletingest.thrift:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.
+ */
+namespace java org.apache.accumulo.core.tabletingest.thrift
+namespace cpp org.apache.accumulo.core.tabletingest.thrift
+
+include "data.thrift"
+include "security.thrift"
+include "client.thrift"
+include "manager.thrift"
+include "master.thrift"
+include "trace.thrift"
+include "tabletserver.thrift"
+
+exception ConstraintViolationException {
+ 1:list<data.TConstraintViolationSummary> violationSummaries
+}
+
+enum TDurability {
+ DEFAULT = 0
+ SYNC = 1
+ FLUSH = 2
+ LOG = 3
+ NONE = 4
+}
+
+service TabletIngestClientService {
Review Comment:
It's a bit tedious to compare these directly with the previous code. It would help to know in advance: did you add/remove anything in here, or simply relocate them from the old consolidated service definitions to the new separate service definitions? I don't see a problem with the relocations in principle, so I'd like to focus on looking at added/removed/substantively changed things, if you can point me to them in advance.
##########
core/src/main/scripts/generate-thrift.sh:
##########
@@ -32,7 +32,7 @@
[[ -z $REQUIRED_THRIFT_VERSION ]] && REQUIRED_THRIFT_VERSION='0.17.0'
[[ -z $INCLUDED_MODULES ]] && INCLUDED_MODULES=()
[[ -z $BASE_OUTPUT_PACKAGE ]] && BASE_OUTPUT_PACKAGE='org.apache.accumulo.core'
-[[ -z $PACKAGES_TO_GENERATE ]] && PACKAGES_TO_GENERATE=(gc master manager tabletserver securityImpl clientImpl dataImpl trace compaction)
+[[ -z $PACKAGES_TO_GENERATE ]] && PACKAGES_TO_GENERATE=(gc master manager tabletserver securityImpl clientImpl dataImpl trace compaction tabletingest tablet tabletscan)
Review Comment:
trace is still listed here. Is that still valid?
--
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