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 2021/02/10 21:08:33 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1923: Update ServerBulkImportStatus for new Bulk import
milleruntime opened a new pull request #1923:
URL: https://github.com/apache/accumulo/pull/1923
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1923: Update ServerBulkImportStatus for new Bulk import. Closes #1510
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1923:
URL: https://github.com/apache/accumulo/pull/1923#discussion_r575508072
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -239,21 +237,21 @@ public void loadFiles(TInfo tinfo, TCredentials credentials, long tid, String di
path = ns.makeQualified(path);
newFileMap.put(new TabletFile(path), mapping.getValue());
}
- var files = newFileMap.keySet();
- bulkImportStatus.updateBulkImportStatus(files, BulkImportState.INITIAL);
+ var fileStream = newFileMap.keySet().stream().map(TabletFile::getPathStr);
+ List<String> files = fileStream.collect(Collectors.toList());
Review comment:
You might be able to get a oneline with a static import of `Collectors.toList()`, if that's what you're going for:
```suggestion
var files = newFileMap.keySet().stream().map(TabletFile::getPathStr).collect(toList());
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1923: Update ServerBulkImportStatus for new Bulk import
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1923:
URL: https://github.com/apache/accumulo/pull/1923#issuecomment-777106043
This PR has conflicts with its target branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1923: Update ServerBulkImportStatus for new Bulk import. Closes #1510
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1923:
URL: https://github.com/apache/accumulo/pull/1923#discussion_r575449708
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -162,6 +164,7 @@
private final TabletServer server;
private final WriteTracker writeTracker = new WriteTracker();
private final RowLocks rowLocks = new RowLocks();
+ private final ServerBulkImportStatus bulkImportStatus = new ServerBulkImportStatus();
Review comment:
This was an unfinished attempt to mirror what was done in `ClientServiceHandler`:
https://github.com/apache/accumulo/blob/09eef7b32da3d1c70709483c5c5a1645e3c64a44/server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java#L78
Turns out it was even easier for `ThriftClientHandler`. Fixed in d3c65cf
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1923: Update ServerBulkImportStatus for new Bulk import
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1923:
URL: https://github.com/apache/accumulo/pull/1923#issuecomment-777422985
> This PR has conflicts with its target branch.
Rebase'd and force pushed branch. Should be good now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #1923: Update ServerBulkImportStatus for new Bulk import. Closes #1510
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1923:
URL: https://github.com/apache/accumulo/pull/1923
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1923: Update ServerBulkImportStatus for new Bulk import. Closes #1510
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1923:
URL: https://github.com/apache/accumulo/pull/1923#discussion_r574806504
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/ServerBulkImportStatus.java
##########
@@ -37,12 +39,22 @@
public void updateBulkImportStatus(List<String> files, BulkImportState state) {
for (String file : files) {
- BulkImportStatus initial = new BulkImportStatus(System.currentTimeMillis(), file, state);
- status.putIfAbsent(file, initial);
- initial = status.get(file);
- if (initial != null) {
- initial.state = state;
- }
+ updateFile(file, state);
+ }
+ }
+
+ private void updateFile(String file, BulkImportState state) {
+ BulkImportStatus initial = new BulkImportStatus(System.currentTimeMillis(), file, state);
+ status.putIfAbsent(file, initial);
+ initial = status.get(file);
+ if (initial != null) {
+ initial.state = state;
+ }
+ }
Review comment:
This could be made to update the concurrent map atomically:
```suggestion
private void updateFile(String file, BulkImportState state) {
status.compute(file, (key, currentStatus) -> {
if (currentStatus == null) {
return new BulkImportStatus(System.currentTimeMillis(), file, state);
}
currentStatus.state = state;
return currentStatus;
});
}
```
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -162,6 +164,7 @@
private final TabletServer server;
private final WriteTracker writeTracker = new WriteTracker();
private final RowLocks rowLocks = new RowLocks();
+ private final ServerBulkImportStatus bulkImportStatus = new ServerBulkImportStatus();
Review comment:
I'm not sure I understand the point of this addition here. I see the three places where you update/remove entries being tracked in this object, but I don't see anywhere where the statuses being tracked in this ever get read and used.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1923: Update ServerBulkImportStatus for new Bulk import. Closes #1510
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1923:
URL: https://github.com/apache/accumulo/pull/1923#discussion_r575152207
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/ServerBulkImportStatus.java
##########
@@ -37,12 +39,22 @@
public void updateBulkImportStatus(List<String> files, BulkImportState state) {
for (String file : files) {
- BulkImportStatus initial = new BulkImportStatus(System.currentTimeMillis(), file, state);
- status.putIfAbsent(file, initial);
- initial = status.get(file);
- if (initial != null) {
- initial.state = state;
- }
+ updateFile(file, state);
+ }
+ }
+
+ private void updateFile(String file, BulkImportState state) {
+ BulkImportStatus initial = new BulkImportStatus(System.currentTimeMillis(), file, state);
+ status.putIfAbsent(file, initial);
+ initial = status.get(file);
+ if (initial != null) {
+ initial.state = state;
+ }
+ }
Review comment:
I figured there was a way but couldn't find the `compute` method. Nice!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org