dlmarion commented on code in PR #4435:
URL: https://github.com/apache/accumulo/pull/4435#discussion_r1555720798
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -87,42 +88,61 @@ public CompactionJobGenerator(PluginEnvironment env,
unknownCompactionServiceErrorCache =
Caches.getInstance().createNewBuilder(CacheName.COMPACTION_SERVICE_UNKNOWN,
false)
.expireAfterWrite(5, TimeUnit.MINUTES).build();
+
+ generateJobsErrorCache =
+
Caches.getInstance().createNewBuilder(CacheName.COMPACTION_SERVICE_UNKNOWN,
false)
+ .expireAfterWrite(5, TimeUnit.MINUTES).build();
+
}
public Collection<CompactionJob> generateJobs(TabletMetadata tablet,
Set<CompactionKind> kinds) {
+ try {
+ Collection<CompactionJob> systemJobs = Set.of();
- // ELASTICITY_TODO do not want user configured plugins to cause exceptions
that prevents tablets
- // from being
- // assigned. So probably want to catch exceptions and log, but not too
spammily OR some how
- // report something
- // back to the manager so it can log.
-
- Collection<CompactionJob> systemJobs = Set.of();
+ if (kinds.contains(CompactionKind.SYSTEM)) {
+ CompactionServiceId serviceId = dispatch(CompactionKind.SYSTEM,
tablet, Map.of());
+ systemJobs = planCompactions(serviceId, CompactionKind.SYSTEM, tablet,
Map.of());
+ }
- if (kinds.contains(CompactionKind.SYSTEM)) {
- CompactionServiceId serviceId = dispatch(CompactionKind.SYSTEM, tablet,
Map.of());
- systemJobs = planCompactions(serviceId, CompactionKind.SYSTEM, tablet,
Map.of());
- }
+ Collection<CompactionJob> userJobs = Set.of();
- Collection<CompactionJob> userJobs = Set.of();
+ if (kinds.contains(CompactionKind.USER) && tablet.getSelectedFiles() !=
null) {
+ var hints =
allExecutionHints.get(tablet.getSelectedFiles().getFateId());
+ if (hints != null) {
+ CompactionServiceId serviceId = dispatch(CompactionKind.USER,
tablet, hints);
+ userJobs = planCompactions(serviceId, CompactionKind.USER, tablet,
hints);
+ }
+ }
- if (kinds.contains(CompactionKind.USER) && tablet.getSelectedFiles() !=
null) {
- var hints = allExecutionHints.get(tablet.getSelectedFiles().getFateId());
- if (hints != null) {
- CompactionServiceId serviceId = dispatch(CompactionKind.USER, tablet,
hints);
- userJobs = planCompactions(serviceId, CompactionKind.USER, tablet,
hints);
+ if (userJobs.isEmpty()) {
+ return systemJobs;
+ } else if (systemJobs.isEmpty()) {
+ return userJobs;
+ } else {
+ var all = new ArrayList<CompactionJob>(systemJobs.size() +
userJobs.size());
+ all.addAll(systemJobs);
+ all.addAll(userJobs);
+ return all;
+ }
+ } catch (Exception e) {
+ // The same code that plans compactions also assigns tablets. The intent
of this catch is
+ // mainly to defend against user plugins called here that throw an
exception from negatively
+ // impacting tablet assignment.
+ String cacheKey = tablet.getTableId() + " " + e.getClass().getName();
+ // This check defends against every tablet in a table having the same
problem and therefore
+ // generating an enormous amount of spam for the logs.
+ var last = generateJobsErrorCache.getIfPresent(cacheKey);
Review Comment:
I think you might be able to just call `Cache.get(Key,Function)` here and
`last` will be `null` if it doesn't exist.
```suggestion
if (generateJobsErrorCache.get(cacheKey, () ->
System.currentTimeMillis) == null) {
```
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -87,42 +88,61 @@ public CompactionJobGenerator(PluginEnvironment env,
unknownCompactionServiceErrorCache =
Caches.getInstance().createNewBuilder(CacheName.COMPACTION_SERVICE_UNKNOWN,
false)
.expireAfterWrite(5, TimeUnit.MINUTES).build();
+
+ generateJobsErrorCache =
+
Caches.getInstance().createNewBuilder(CacheName.COMPACTION_SERVICE_UNKNOWN,
false)
+ .expireAfterWrite(5, TimeUnit.MINUTES).build();
+
}
public Collection<CompactionJob> generateJobs(TabletMetadata tablet,
Set<CompactionKind> kinds) {
+ try {
+ Collection<CompactionJob> systemJobs = Set.of();
- // ELASTICITY_TODO do not want user configured plugins to cause exceptions
that prevents tablets
- // from being
- // assigned. So probably want to catch exceptions and log, but not too
spammily OR some how
- // report something
- // back to the manager so it can log.
-
- Collection<CompactionJob> systemJobs = Set.of();
+ if (kinds.contains(CompactionKind.SYSTEM)) {
+ CompactionServiceId serviceId = dispatch(CompactionKind.SYSTEM,
tablet, Map.of());
+ systemJobs = planCompactions(serviceId, CompactionKind.SYSTEM, tablet,
Map.of());
+ }
- if (kinds.contains(CompactionKind.SYSTEM)) {
- CompactionServiceId serviceId = dispatch(CompactionKind.SYSTEM, tablet,
Map.of());
- systemJobs = planCompactions(serviceId, CompactionKind.SYSTEM, tablet,
Map.of());
- }
+ Collection<CompactionJob> userJobs = Set.of();
- Collection<CompactionJob> userJobs = Set.of();
+ if (kinds.contains(CompactionKind.USER) && tablet.getSelectedFiles() !=
null) {
+ var hints =
allExecutionHints.get(tablet.getSelectedFiles().getFateId());
+ if (hints != null) {
+ CompactionServiceId serviceId = dispatch(CompactionKind.USER,
tablet, hints);
+ userJobs = planCompactions(serviceId, CompactionKind.USER, tablet,
hints);
+ }
+ }
- if (kinds.contains(CompactionKind.USER) && tablet.getSelectedFiles() !=
null) {
- var hints = allExecutionHints.get(tablet.getSelectedFiles().getFateId());
- if (hints != null) {
- CompactionServiceId serviceId = dispatch(CompactionKind.USER, tablet,
hints);
- userJobs = planCompactions(serviceId, CompactionKind.USER, tablet,
hints);
+ if (userJobs.isEmpty()) {
+ return systemJobs;
+ } else if (systemJobs.isEmpty()) {
+ return userJobs;
+ } else {
+ var all = new ArrayList<CompactionJob>(systemJobs.size() +
userJobs.size());
+ all.addAll(systemJobs);
+ all.addAll(userJobs);
+ return all;
+ }
+ } catch (Exception e) {
+ // The same code that plans compactions also assigns tablets. The intent
of this catch is
+ // mainly to defend against user plugins called here that throw an
exception from negatively
+ // impacting tablet assignment.
+ String cacheKey = tablet.getTableId() + " " + e.getClass().getName();
+ // This check defends against every tablet in a table having the same
problem and therefore
+ // generating an enormous amount of spam for the logs.
+ var last = generateJobsErrorCache.getIfPresent(cacheKey);
+ if (last == null) {
+ log.error(
+ "Failed to generate compaction jobs for {}. Other tablets may be
experiencing the"
+ + " same error, this log message is temporarily suppressed for
the entire table.",
+ tablet.getExtent(), e);
+ generateJobsErrorCache.put(cacheKey, System.currentTimeMillis());
Review Comment:
Remove this if you apply my suggestion above.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
